2019-04-25 05:53:48

by Eugeniu Rosca

[permalink] [raw]
Subject: Re: [PATCH 16/28] staging: most: sound: call snd_card_new with struct device

Hi Christian,

On Tue, 08 May 2018 02:46:44 -0700, Christian Gromm wrote:
> This patch is needed as function snd_card_new needs a valid
> parent device. Passing a NULL pointer leads to kernel Ooops.
>
> Signed-off-by: Christian Gromm <[email protected]>
> ---
> drivers/staging/most/core.h | 1 +
> drivers/staging/most/sound/sound.c | 2 +-
> drivers/staging/most/usb/usb.c | 1 +
> 3 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/most/core.h b/drivers/staging/most/core.h
> index 7a3c70bead19..64cc02f161e7 100644
> --- a/drivers/staging/most/core.h
> +++ b/drivers/staging/most/core.h
> @@ -230,6 +230,7 @@ struct mbo {
> */
> struct most_interface {
> struct device dev;
> + struct device *driver_dev;
> struct module *mod;
> enum most_interface_type interface;
> const char *description;
> diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
> index 18f722410a63..04c18323c2ea 100644
> --- a/drivers/staging/most/sound/sound.c
> +++ b/drivers/staging/most/sound/sound.c
> @@ -590,7 +590,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
> if (ret < 0)
> return ret;
>
> - ret = snd_card_new(NULL, -1, card_name, THIS_MODULE,
> + ret = snd_card_new(&iface->dev, -1, card_name, THIS_MODULE,
> sizeof(*channel), &card);
> if (ret < 0)
> return ret;
> diff --git a/drivers/staging/most/usb/usb.c b/drivers/staging/most/usb/usb.c
> index 5ed1dccc0839..f18726049528 100644
> --- a/drivers/staging/most/usb/usb.c
> +++ b/drivers/staging/most/usb/usb.c
> @@ -1043,6 +1043,7 @@ hdm_probe(struct usb_interface *interface, const struct usb_device_id *id)
> mdev->link_stat_timer.expires = jiffies + (2 * HZ);
>
> mdev->iface.mod = hdm_usb_fops.owner;
> + mdev->iface.driver_dev = &interface->dev;
> mdev->iface.interface = ITYPE_USB;
> mdev->iface.configure = hdm_configure_channel;
> mdev->iface.request_netinfo = hdm_request_netinfo;

Just for your information, when mapping commits from vanilla to those
from https://github.com/microchip-ais/linux/commits/mld-1.8.0, we've
stumbled upon some subtle but striking difference between mld-1.8.0
commit [0] and v4.18-rc1 commit [1]. The latter looks like an upstreamed
version of the former. However, while commit [0] creates a new 'dev'
member in 'struct most_interface' and uses it consistently, commit [1]
creates 'driver_dev' and uses it intermixed with 'dev'.

Since we don't use aim-sound, we just signal this feedback to you
as FWIW without sending a patch (which we can't test).

[0] https://github.com/microchip-ais/linux/commit/2fef0f89f04703
("staging: most: add struct device to most interface")
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=69c90cf1b2faf5
("staging: most: sound: call snd_card_new with struct device")

--
Best regards,
Eugeniu.


2019-04-29 09:31:26

by Christian Gromm

[permalink] [raw]
Subject: Re: [PATCH 16/28] staging: most: sound: call snd_card_new with struct device

On Mi, 2019-04-24 at 20:50 +0200, Eugeniu Rosca wrote:
> External E-Mail
>
>
> Hi Christian,
>
> On Tue, 08 May 2018 02:46:44 -0700, Christian Gromm wrote:
> >
> >     This patch is needed as function snd_card_new needs a valid
> >     parent device. Passing a NULL pointer leads to kernel Ooops.
> >     
> >     Signed-off-by: Christian Gromm <[email protected]>
> > ---
> >  drivers/staging/most/core.h        | 1 +
> >  drivers/staging/most/sound/sound.c | 2 +-
> >  drivers/staging/most/usb/usb.c     | 1 +
> >  3 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/most/core.h
> > b/drivers/staging/most/core.h
> > index 7a3c70bead19..64cc02f161e7 100644
> > --- a/drivers/staging/most/core.h
> > +++ b/drivers/staging/most/core.h
> > @@ -230,6 +230,7 @@ struct mbo {
> >   */
> >  struct most_interface {
> >   struct device dev;
> > + struct device *driver_dev;
> >   struct module *mod;
> >   enum most_interface_type interface;
> >   const char *description;
> > diff --git a/drivers/staging/most/sound/sound.c
> > b/drivers/staging/most/sound/sound.c
> > index 18f722410a63..04c18323c2ea 100644
> > --- a/drivers/staging/most/sound/sound.c
> > +++ b/drivers/staging/most/sound/sound.c
> > @@ -590,7 +590,7 @@ static int audio_probe_channel(struct
> > most_interface *iface, int channel_id,
> >   if (ret < 0)
> >   return ret;
> >  
> > - ret = snd_card_new(NULL, -1, card_name, THIS_MODULE,
> > + ret = snd_card_new(&iface->dev, -1, card_name,
> > THIS_MODULE,
> >      sizeof(*channel), &card);
> >   if (ret < 0)
> >   return ret;
> > diff --git a/drivers/staging/most/usb/usb.c
> > b/drivers/staging/most/usb/usb.c
> > index 5ed1dccc0839..f18726049528 100644
> > --- a/drivers/staging/most/usb/usb.c
> > +++ b/drivers/staging/most/usb/usb.c
> > @@ -1043,6 +1043,7 @@ hdm_probe(struct usb_interface *interface,
> > const struct usb_device_id *id)
> >   mdev->link_stat_timer.expires = jiffies + (2 * HZ);
> >  
> >   mdev->iface.mod = hdm_usb_fops.owner;
> > + mdev->iface.driver_dev = &interface->dev;
> >   mdev->iface.interface = ITYPE_USB;
> >   mdev->iface.configure = hdm_configure_channel;
> >   mdev->iface.request_netinfo = hdm_request_netinfo;
> Just for your information, when mapping commits from vanilla to those
> from https://github.com/microchip-ais/linux/commits/mld-1.8.0, we've
> stumbled upon some subtle but striking difference between mld-1.8.0
> commit [0] and v4.18-rc1 commit [1]. The latter looks like an
> upstreamed
> version of the former. However, while commit [0] creates a new 'dev'
> member in 'struct most_interface' and uses it consistently, commit
> [1]
> creates 'driver_dev' and uses it intermixed with 'dev'.

This looks sort of suspicious. I'll check it.

Thanks for letting me know.

Regards,
Chris

>
> Since we don't use aim-sound, we just signal this feedback to you
> as FWIW without sending a patch (which we can't test).
>
> [0] https://github.com/microchip-ais/linux/commit/2fef0f89f04703
>     ("staging: most: add struct device to most interface")
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.gi
> t/commit/?id=69c90cf1b2faf5
>     ("staging: most: sound: call snd_card_new with struct device")
>