Re: [PATCH v3 2/2] mcb: use short version for function pointer for mcb_free_bus

[Rodríguez Barbarin, José Javier]

On Thu, 2023-10-05 at 09:46 +0200, gregkh@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxx wrote:
> > > On Wed, Sep 06, 2023 at 11:49:28AM +0000, Rodríguez Barbarin,
> > > José
> > > Javier wrote:
> > > > From: Jorge Sanjuan Garcia <jorge.sanjuangarcia@xxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxx>
> > > > Co-developed-by: Jose Javier Rodriguez Barbarin
> > > > <JoseJavier.Rodriguez@xxxxxxxxxx>
> > > > Signed-off-by: Jose Javier Rodriguez Barbarin
> > > > <JoseJavier.Rodriguez@xxxxxxxxxx>
> > > > ---
> > > >  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