[alsa-devel] [RFC] ucb1400 touchscreen, irq auto probing and ac97 with its private field

[Takashi Iwai]

At Fri, 25 Apr 2008 12:54:29 +0200,
Sebastian Siewior wrote:
> 
> * Jaroslav Kysela | 2008-04-25 10:23:51 [+0200]:
> 
> >On Fri, 25 Apr 2008, Takashi Iwai wrote:
> >
> >> At Fri, 25 Apr 2008 09:35:47 +0200 (CEST),
> >> Jaroslav Kysela wrote:
> >> > 
> >> > On Fri, 25 Apr 2008, Takashi Iwai wrote:
> >> > 
> >> > > > Sure. I applied the simple 'void *device_private_data' patch, because 
> >> > > > current usage request is really trivial. We can implement complex code to 
> >> > > > handle data for multiple "extra" devices on AC97 bus later.
> >> > > 
> >> > > Actually, it's not "used" yet.  The ucb1000 reads the data but no one
> >> > > stores yet.  And, if its usage request is trivial, we should use "int
> >> > 
> >> > Yes, I hope that the appropriate initialization code will be added to SoC 
> >> > drivers, too.
> >> > 
> >> > > irq" as in the original patch instead of void data and cast.
> >> > 
> >> > But other SoC (or other) drivers might want to pass to extra devices on 
> >> > AC97 bus something different or more complex. Mark Brown already noted
> >> > that. I would keep it as 'void *'.
> >> 
> >> That's the very problem I've been trying to point out.
> >> The void pointer is good if the same driver assigns and casts.  But,
> >> in this case, the allocator and the receiver are different.  Thus,
> >> there is no guarantee that the data type is what you want.  OTOH, if
> >> it's "int irq", this is crystal clear.
> >> 
> >> So, in short:
> >> 
> >> - if only one device needs such data, it should be a strong type like
> >>   "int irq" anyway -- no extra need to cast to void pointer
> >> - if multiple devices need such a pass-away mechanism, then they can
> >>   crash because you have no data type check.  The void pointer is
> >>   dangerous for multiple devices.
> >
> >I see. In this case, I would propose to add a 32-bit "magic" at the 
> >start of 'void *' data. How about this modification:
> >
> >diff -r e2ff47e8771b include/ac97_codec.h
> >--- a/include/ac97_codec.h	Fri Apr 25 08:29:05 2008 +0200
> >+++ b/include/ac97_codec.h	Fri Apr 25 10:22:00 2008 +0200
> >@@ -407,6 +407,9 @@
> > #define AC97_RATES_MIC_ADC	4
> > #define AC97_RATES_SPDIF	5
> > 
> >+/* device private data magic number */
> >+#define AC97_PDEVMAGIC_IRQ	0x20495251	/* in ASCII: <space>IRQ */
> >+
> > /*
> >  *
> >  */
> >@@ -545,6 +547,11 @@ static inline int ac97_can_spdif(struct 
> > static inline int ac97_can_spdif(struct snd_ac97 * ac97)
> > {
> > 	return (ac97->ext_id & AC97_EI_SPDIF) != 0;
> >+}
> >+static inline int ac97_check_pdevdata_magic(struct snd_ac97 * ac97, unsigned int magic)
> >+{
> >+	return (ac97->device_private_data &&
> >+		*((unsigned int *)ac97->device_private_data) == magic);
> > }
> > 
> > /* functions */
> >diff -r e2ff47e8771b kernel/drivers/input/touchscreen/ucb1400_ts.c
> >--- a/kernel/drivers/input/touchscreen/ucb1400_ts.c	Fri Apr 25 08:29:05 2008 +0200
> >+++ b/kernel/drivers/input/touchscreen/ucb1400_ts.c	Fri Apr 25 10:22:00 2008 +0200
> >@@ -492,14 +492,14 @@ static int ucb1400_ts_probe(struct devic
> > 		goto err_free_devs;
> > 	}
> > 
> >-	if (!ucb->ac97->device_private_data) {
> >+	if (!ac97_check_pdevdata_magic(usb->ac97, AC97_PDEVMAGIC_IRQ)) {
> > 		error = ucb1400_detect_irq(ucb);
> > 		if (error) {
> > 			printk(KERN_ERR "UCB1400: IRQ probe failed\n");
> > 			goto err_free_devs;
> > 		}
> > 	} else {
> >-		ucb->irq = (int) ucb->ac97->device_private_data;
> >+		ucb->irq = ((int *) ucb->ac97->device_private_data)[1];
> > 	}
> > 
> > 	error = request_irq(ucb->irq, ucb1400_hard_irq, IRQF_TRIGGER_RISING,
> >
> This is getting beyond what I planned. Now I have to allocate a struct
> and I don't like the void * to int casts. I just did it once to save an
> allocation of 4 bytes for my int *. 
> Why is it a problem to keep an anonymous struct?

You describe in your text below :)

> If some one uses a
> wrong struct than it crashes immediatelly or bails out because
> 0x20495251 is way too large be an IRQ. Putting that magic and casting
> for every single possible data blows code for no good reason. Don't
> recover from errors which should not have happen, solve them at root
> level not where the leaves are.

The root level of the problem is that you pass the anonymous data.
It _IS_ unsafe and wrong unless handled properly.

> What I intended in first place is to allocate a private field in the bus
> struct so can pass informations to the lower driver.

As mentioned in my earlier mail, I'm fine with your first patch.  The
problem occurs when we generalize it.

> If you need
> multiple arguments, create your own struct put it in the void * slot,
> your driver knows what to do.

Your driver does _not_ know what type it is because the data isn't
created by your driver but the controller driver.  And, it's free to
attach your driver to any controller.


thanks,

Takashi