| Message ID | 1422872678-4815-4-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/02/2015 11:24 AM, Takashi Iwai wrote: > diff --git a/sound/core/init.c b/sound/core/init.c > index 074875d68c15..2f730efe97b6 100644 > --- a/sound/core/init.c > +++ b/sound/core/init.c > @@ -157,6 +157,25 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int), > return mask; /* unchanged */ > } > > +static void default_release(struct device *dev) > +{ > +} A empty release callback is pretty much always wrong and typically causes use-after-free bugs. It might be correct in this case, but there should at least be a comment explaining why it is correct. And on the long run things should probably be re-factored to do all memory freeing in a subdevice specific release function. - Lars
At Mon, 02 Feb 2015 14:31:40 +0100, Lars-Peter Clausen wrote: > > On 02/02/2015 11:24 AM, Takashi Iwai wrote: > > diff --git a/sound/core/init.c b/sound/core/init.c > > index 074875d68c15..2f730efe97b6 100644 > > --- a/sound/core/init.c > > +++ b/sound/core/init.c > > @@ -157,6 +157,25 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int), > > return mask; /* unchanged */ > > } > > > > +static void default_release(struct device *dev) > > +{ > > +} > > A empty release callback is pretty much always wrong and typically causes > use-after-free bugs. It might be correct in this case, but there should at > least be a comment explaining why it is correct. Right, a bit more explanation would be better: we have already dev_free callback in snd_device chain, and this would do almost all job. That's why we can leave empty as default there. > And on the long run things > should probably be re-factored to do all memory freeing in a subdevice > specific release function. This patchset is one step toward the simplification, indeed. But, what do you have in mind when you write "a subsystem specific release function"? thanks, Takashi
On 02/02/2015 02:40 PM, Takashi Iwai wrote: [...] >> And on the long run things >> should probably be re-factored to do all memory freeing in a subdevice >> specific release function. > > This patchset is one step toward the simplification, indeed. > > But, what do you have in mind when you write "a subsystem specific > release function"? "subdev specific". What you already do for e.g. hwdep and MIDI, but for all of the subdevice types, so the empty default function is no longer needed. The kfree(container_of(...)) stuff. - Lars
At Mon, 02 Feb 2015 14:43:02 +0100, Lars-Peter Clausen wrote: > > On 02/02/2015 02:40 PM, Takashi Iwai wrote: > [...] > >> And on the long run things > >> should probably be re-factored to do all memory freeing in a subdevice > >> specific release function. > > > > This patchset is one step toward the simplification, indeed. > > > > But, what do you have in mind when you write "a subsystem specific > > release function"? > > "subdev specific". What you already do for e.g. hwdep and MIDI, but for all > of the subdevice types, so the empty default function is no longer needed. > > The kfree(container_of(...)) stuff. I see. Yes, the whole device_free stuff should be moved to each release function ideally. But as of now, doing it is buggy because of the lack of kobject dependency chain between card and subsystem objects. We'd need to manage the dependency chain among card and subsystems, not only the refcount of opened subsystem files. Maybe I'll start working on it with the next patchset once after this patchset is merged. Another TODO is to rewrite dev_disconnect and dev_regsiter call chain with another method, e.g. the standard notifier chain. Then we can get rid of snd_device stuff. Takashi
diff --git a/include/sound/core.h b/include/sound/core.h index 39d14234961d..de7a878217d7 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -207,6 +207,8 @@ extern struct class *sound_class; void snd_request_card(int card); +void snd_device_initialize(struct device *dev, struct snd_card *card); + int snd_register_device_for_dev(int type, struct snd_card *card, int dev, const struct file_operations *f_ops, void *private_data, struct device *device, diff --git a/sound/core/init.c b/sound/core/init.c index 074875d68c15..2f730efe97b6 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -157,6 +157,25 @@ static int get_slot_from_bitmask(int mask, int (*check)(struct module *, int), return mask; /* unchanged */ } +static void default_release(struct device *dev) +{ +} + +/** + * snd_device_initialize - Initialize struct device for sound devices + * @dev: device to initialize + * @card: card to assign, optional + */ +void snd_device_initialize(struct device *dev, struct snd_card *card) +{ + device_initialize(dev); + if (card) + dev->parent = &card->card_dev; + dev->class = sound_class; + dev->release = default_release; +} +EXPORT_SYMBOL_GPL(snd_device_initialize); + static int snd_card_do_free(struct snd_card *card); static const struct attribute_group *card_dev_attr_groups[];
Introduce a new helper function snd_device_initialize() to initialize the device object for sound devices. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- include/sound/core.h | 2 ++ sound/core/init.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+)