Re: [PATCH v7 4/9] driver: core: allow modifying device_links flags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 26, 2024 at 6:24 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Fri, 2024-01-26 at 09:13 +0100, Nuno Sá wrote:
> > On Thu, 2024-01-25 at 16:50 -0800, Saravana Kannan wrote:
> > > On Thu, Jan 25, 2024 at 12:11 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> > > >
> > > >
> > > > Hi Saravana,
> > > >
> > > > Thanks for your feedback,
> > > >
> > > > On Wed, 2024-01-24 at 19:21 -0800, Saravana Kannan wrote:
> > > > > On Tue, Jan 23, 2024 at 7:14 AM Nuno Sa via B4 Relay
> > > > > <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > > > > >
> > > > > > If a device_link is previously created (eg: via
> > > > > > fw_devlink_create_devlink()) before the supplier + consumer are both
> > > > > > present and bound to their respective drivers, there's no way to set
> > > > > > DL_FLAG_AUTOREMOVE_CONSUMER anymore while one can still set
> > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER. Hence, rework the flags checks to allow
> > > > > > for DL_FLAG_AUTOREMOVE_CONSUMER in the same way
> > > > > > DL_FLAG_AUTOREMOVE_SUPPLIER is done.
> > > > >
> > > > > Curious, why do you want to set DL_FLAG_AUTOREMOVE_CONSUMER?
> > > > > Especially if fw_devlink already created the link? You are effectively
> > > > > trying to delete the link fw_devlink created if any of your devices
> > > > > unbind.
> > > > >
> > > >
> > > > Well, this is still useful in the modules case as the link will be relaxed
> > > > after
> > > > all devices are initialized and that will already clear AUTOPROBE_CONSUMER
> > > > AFAIU. But, more importantly, if I'm not missing anything, in [1],
> > > > fw_devlinks
> > > > will be dropped after the consumer + supplier are bound which means I
> > > > definitely
> > > > want to create a link between my consumer and supplier.
> > > >
> > > > FWIW, I was misunderstanding things since I thought
> > > > DL_FLAG_AUTOREMOVE_CONSUMER
> > > > was needed to make sure the consumer is unbound before the supplier. But
> > > > for
> > > > that I think I can even pass 0 in the flags as I only need the link to be
> > > > MANAGED. Still, I think having DL_FLAG_AUTOREMOVE_CONSUMER makes sense.
> > >
> > > As you noticed, your understanding of DL_FLAG_AUTOREMOVE_CONSUMER is
> > > not correct. There's almost never a good reason to drop a device link.
> > > Even when a device/driver are unbound, we still want future probe
> > > attempts to make use of the dependency info and block a device from
> > > probing if the supplier hasn't probed.
> > >
> >
> > Yeah that makes sense and is making me thinking that I should change my call
> > (in
> > patch 7 to use the MANAGED flag instead of AUTOREMOVE_CONSUMER). Sure,
> > AUTOREMOVE_CONSUMER won't matter most cases but if someone disables
> > fw_devlinks
> > then it matters.

I don't fully understand the patch series, but what exactly are you
gaining by adding device links explicitly. I'd rather that every
driver didn't explicitly create a device link. That's just a lot of
useless code in most cases and we shouldn't have useless code lying
around. If someone does fw_devlink=off and you don't create a device
link explicitly, what's the worst that's going to happen? Useless
deferred probes? fw_devlink is really only there as a debug tool at
this point -- I don't think you need to worry about people setting it
to off/permissive.

> >
>
> Yeah, just realized MANAGED is not a valid flag one can pass to
> device_link_add() :)

If you don't pass the STATELESS flag, a link is assumed to be MANAGED.
So, you can still create managed device links.

-Saravana





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux