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