[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4374befb2cb49d953c9c3d0f142f6b8794b80843.camel@duagon.com>
Date: Fri, 6 Oct 2023 12:32:13 +0000
From: Rodríguez Barbarin, José Javier
<josejavier.rodriguez@...gon.com>
To: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: "jirislaby@...nel.org" <jirislaby@...nel.org>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"jth@...nel.org" <jth@...nel.org>,
Sanjuán García, Jorge
<Jorge.SanjuanGarcia@...gon.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"morbidrsa@...il.com" <morbidrsa@...il.com>
Subject: Re: [PATCH v3 2/2] mcb: use short version for function pointer for
mcb_free_bus
On Thu, 2023-10-05 at 09:46 +0200, gregkh@...uxfoundation.org wrote:
> On Fri, Sep 22, 2023 at 12:28:14PM +0000, Rodríguez Barbarin, José
> Javier wrote:
> > On Wed, 2023-09-20 at 15:18 +0200,
> > gregkh@...uxfoundation.org wrote:
> > > On Wed, Sep 06, 2023 at 11:49:28AM +0000, Rodríguez Barbarin,
> > > José
> > > Javier wrote:
> > > > From: Jorge Sanjuan Garcia <jorge.sanjuangarcia@...gon.com>
> > > >
> > > > Just a style change so that the device release callbacks are
> > > > defined
> > > > in the same way for devices in mcb_bus and mcb_device.
> > > >
> > > > Signed-off-by: Jorge Sanjuan Garcia
> > > > <jorge.sanjuangarcia@...gon.com>
> > > > Co-developed-by: Jose Javier Rodriguez Barbarin
> > > > <JoseJavier.Rodriguez@...gon.com>
> > > > Signed-off-by: Jose Javier Rodriguez Barbarin
> > > > <JoseJavier.Rodriguez@...gon.com>
> > > > ---
> > > > drivers/mcb/mcb-core.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mcb/mcb-core.c b/drivers/mcb/mcb-core.c
> > > > index 0cac5bead84f..5c6157b0db75 100644
> > > > --- a/drivers/mcb/mcb-core.c
> > > > +++ b/drivers/mcb/mcb-core.c
> > > > @@ -288,7 +288,7 @@ struct mcb_bus *mcb_alloc_bus(struct device
> > > > *carrier)
> > > > bus->dev.parent = carrier;
> > > > bus->dev.bus = &mcb_bus_type;
> > > > bus->dev.type = &mcb_carrier_device_type;
> > > > - bus->dev.release = &mcb_free_bus;
> > > > + bus->dev.release = mcb_free_bus;
> > >
> > > But you aren't fixing the root cause here of an incorrect pointer
> > > being
> > > passed to this function, right?
> > >
> > > Yes, removing the single variable is nicer, so the crash doesn't
> > > happen,
> > > but you are still passing the wrong pointer around, so why not
> > > fix
> > > that?
> > >
> >
> > > thanks,
> > >
> > > greg k-h
> >
> > The pointer to struct device in function __mcb_bus_add_devices()
> > always
> > was the correct one. The problem came when calling to function
> > to_mcb_device() which was hapenning even for the case of struct
> > device
> > pointer being a member of struct mcb_bus.
> >
> > Removing the need for this conversion makes the function generic so
> > that it will work for both mcb_device and mcb_bus structs. This
> > already
> > fixes the crash as no member overlapping will occur (is_added in
> > mcb_device struct and bus_nr in mcb_bus struct).
> >
> > We belive the pointer is the correct one and this patch series was
> > actually fixing the root cause of the crash. What do you mean by
> > "passing the wrong pointer around"? are we missing something?
>
> Ok, I understand now, yes, this looks correct.
>
> But, the function mcb_bus_add_devices() seems odd to me. You are
> passing in a parameter that you are never using, so why have it at
> all?
> You are implying that you only have one bus, yet you are ignoring the
> bus sent to you?
>
> This still seems wrong.
>
> I'll queue up this series as it obviously fixes a bug, but more needs
> to
> be done here.
>
> thanks,
>
> greg k-h
Thank you Greg, I will think about your suggestions and as soon as I
have a new patch that fixes it, I will send it to you.
Regards
Powered by blists - more mailing lists