Re: [PATCH] scsi: core: Don't free dev_name() manually

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Sun, Jan 18, 2026 at 09:45:26AM -0500, James Bottomley wrote:
> On Sun, 2026-01-18 at 03:32 +0800, Tzung-Bi Shih wrote:
> > > scsi_host_alloc() is designed to hold initial reference count of
> > > `&shost->shost_gendev` and `&shost->shost_dev`.  In the error
> > > handling paths [1], only drop a reference count to `&shost-
> > > >shost_gendev` is sufficient as scsi_host_dev_release() will be
> > > called and the reference count of `&shost->shost_dev` should be
> > > dropped at that time.
> > > 
> > > Drivers shouldn't need to free the device name and hold a reference
> > > count to its parent device as the driver core automatically handles
> > > that.  Remove them.
> > > 
> > > [1] Either at "fail" label in scsi_host_alloc() or in SCSI drivers
> > > that
> > >     a subsequent scsi_add_host{,_with_dma}() fails.
> 
> This commit description seems to bear almost no relation to what's
> going on in the commit ... please describe why you're doing what you're
> doing (like eliminating the class based device get and the parent get
> and, apparently, trying to flatten the device tree in the host).

An attempt to fix in v2[2].

[2] https://lore.kernel.org/all/20260119142306.33676-1-tzungbi@kernel.org/T/#u.

> 
> > > 
> > > Fixes: b49493f99690 ("Fix a memory leak in
> > > scsi_host_dev_release()")
> > > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> > > ---
> > >  drivers/scsi/hosts.c | 16 +++++-----------
> > >  1 file changed, 5 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > > index 1b3fbd328277..b88d553cdde6 100644
> > > --- a/drivers/scsi/hosts.c
> > > +++ b/drivers/scsi/hosts.c
> > > @@ -55,7 +55,6 @@ static DEFINE_IDA(host_index_ida);
> > >  
> > >  static void scsi_host_cls_release(struct device *dev)
> > >  {
> > > - put_device(&class_to_shost(dev)->shost_gendev);
> > >  }
> 
> An empty release function can simply become a NULL pointer.  I assume
> the reason this one doesn't is because the device core will complain if
> a device has no release function ... in which case a comment why we
> don't need one should be here.

Fixed in v2.

> But there's a reason for the warning: a bigger problem with this is the
> parenting goes
> 
> shost_dev -> shost_gendev -> underlying device
> 
> And shost_dev is visible in the sysfs tree so it could possibly by held
> in place by user space.  If that happens, since you've now removed the
> reference it took on shost_gendev, what stops shost_gendev (and the
> rest of the host) being freed?
> 
> > >  
> > >  static struct class shost_class = {
> > > @@ -279,11 +278,9 @@ int scsi_add_host_with_dma(struct Scsi_Host
> > > *shost, struct device *dev,
> > >   goto out_disable_runtime_pm;
> > >  
> > >   scsi_host_set_state(shost, SHOST_RUNNING);
> > > - get_device(shost->shost_gendev.parent);
> 
> We need a reference to the parent to prevent surprise removal ... where
> else is the reference held?

It looks to me the same question as above.  IIUC, device_add() holds a
reference count to its parent[3].  Drivers don't need to do it explicitly.

[3] https://elixir.bootlin.com/linux/v6.18/source/drivers/base/core.c#L3612

> 
> > >  
> > >   device_enable_async_suspend(&shost->shost_dev);
> > >  
> > > - get_device(&shost->shost_gendev);
> 
> I assume this is matched to the class dev_release which is gone?  If
> so, say in the commit message.

Fixed in v2.