Re: [PATCH] pata_legacy: Allow disabling of legacy PATA device probes on non-PCI systems

[Tejun Heo <tj@kernel.org>]

Hello,

On Mon, Jan 23, 2017 at 03:36:18PM -0800, Gwendal Grignou wrote:
> > Can you please explain why the libata transport code is explicitly
> > pinning the parent device and then putting it in the release methods?
> > device_add/del() already gets and puts the parent, so I don't get why
> > this part is necessary.  Is this intentional?
>
> It was intentional. When I wrote the code, I used other transport as
> inspiration, in particular drivers/scsi/scsi_transport_sas.c.
> For instance, in sas_port_alloc, "port->dev.parent =
> get_device(parent);", undo in sas_port_release().
> From your debug code, in case of ATA at least, it looks unnecessary.

kobjects release their parents on kobject_del() rather than release,
which I don't think is a good design, so if there are code paths which
try to walk up the hierarchy after being deleted but before released,
having the extra ref around release does make sense.  I don't know
whether that's the case for the transport code tho.

Anyways, I was curious but this is a bit tangential to the issue.
This code does make the ata device hang around till the transport
kobject is released and the problem is caused by the transport kobject
not being released, so there's linkage there, but we wanna find out
why the transport kobject isn't being released and fix it.

> Moreover, in the error path of ata_XXX_add, a put_device(&link->tdev)
> Also,How ata_tport_add() increase the reference twice before calling
> device_add()?

Dunno, attribute additions?

> > Also, it looks like there is a ref leak on the transport device
> > itself.  Its release function never gets called and thus the parent
> > device (ata_port) stays pinned too.
>
> That's the root cause of pata port not released. Can we run your debug
> code one more time, enabling ap->tdev.kobj.release_debug?

Yeah, I'll update the patch.

Thanks.

-- 
tejun