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 Thu, Jan 25, 2024 at 4:31 PM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Thu, 2024-01-25 at 09:14 +0100, Nuno Sá 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.
> >
>
> Ok, so to add a bit more on this, there are two cases:
>
> 1) Both sup and con are modules and after boot up, the link is relaxed and thus
> turned into a sync_state_only link. That means the link will be deleted anyways
> and AUTOPROBE_CONSUMER is already cleared by the time we try to change the link.
>
> 2) The built-in case where the link is kept as created by fw_devlink and this
> patch effectively clears AUTOPROBE_CONSUMER.
>
> Given the above, not sure what's the best option. I can think of 4:
>
> 1) Drop this patch and leave things as they are. DL_FLAG_AUTOREMOVE_CONSUMER is
> pretty much ignored in my call but it will turn the link in a MANAGED one and
> clear SYNC_STATE_ONLY. I could very well just pass 0 in the flags as
> DL_FLAG_AUTOREMOVE_CONSUMER is always ignored;
>
> 2) Rework this patch so we can still change an existing link to accept
> DL_FLAG_AUTOREMOVE_CONSUMER (in the modules case for example).
>
> However, instead of clearing AUTOPROBE_CONSUMER, I would add some checks so if
> flags have one of DL_FLAG_AUTOREMOVE_SUPPLIER or DL_FLAG_AUTOREMOVE_CONSUMER and
> AUTOPROBE_CONSUMER is already set, we ignore them. In fact, right now, I think
> one could pass DL_FLAG_AUTOREMOVE_SUPPLIER and link->flags ends ups with
> AUTOREMOVE_SUPPLIER | AUTOPROBE_CONSUMER which in theory is not allowed...

No, because DL_FLAG_AUTOREMOVE_SUPPLIER is only added to the link
flags if DL_FLAG_AUTOREMOVE_CONSUMER is already set in there and the
former replaces the latter.

Now, DL_FLAG_AUTOREMOVE_CONSUMER cannot be set in the link flags if
AUTOPROBE_CONSUMER is set in there.

> 3) Keep it as-is... This one is likely a NACK as I'm getting the feeling that
> clearing stuff that might have been created by fw_devlinks is probably a no-go.
>
> Let me know your thoughts...

If the original creator of the link didn't indicate either
DL_FLAG_AUTOREMOVE_CONSUMER, or DL_FLAG_AUTOREMOVE_SUPPLIER, they are
expected to need the link to stay around until it is explicitly
deleted.

Therefore adding any of these flags for an existing link where they
both are unset would be a mistake, because it would effectively cause
the link to live shorter than expected by the original creator and
that might lead to correctness issues.

Thanks!





[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