Re: [PATCH 2/4] usb: typec: tipd: add function to request firmware

[Heikki Krogerus]

On Tue, Dec 12, 2023 at 03:41:35PM +0100, Javier Carrasco wrote:
> 
> 
> On 12.12.23 15:15, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Fri, Dec 08, 2023 at 07:58:52PM +0100, Javier Carrasco wrote:
> >> Hi Heikki,
> >>
> >> On 08.12.23 15:55, Heikki Krogerus wrote:
> >>
> >>>> +	ret = request_firmware(fw, firmware_name, tps->dev);
> >>>> +	if (ret) {
> >>>> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
> >>>> +		/* probe deferring in case the file system is not ready */
> >>>> +		return (ret == -ENOENT) ? -EPROBE_DEFER : ret;
> >>>
> >>> It's more likely that the firmware really isn't available, and it will
> >>> never be available in this case. I think there is only one place in
> >>> kernel where failing request_firmware() can lead to deferred probe
> >>> (drivers/tee/optee/smc_abi.c) and there the code can actually see the
> >>> system state - that's actually the condition.
> >>>
> >>> So just return dev_err_probe() here:
> >>>
> >>> 	ret = request_firmware(fw, firmware_name, tps->dev);
> >>> 	if (ret)
> >>>                 return dev_err_probe(tps->dev, ret, "failed to retrieve \"%s\"", firmware_name);
> >>>
> >> Thank you for your feedback.
> >>
> >> This solution arose from a real use case: in the system I am using to
> >> test the tps65987d, the filesystem is not ready when the probe function
> >> is called. If I just return on -ENOENT, the device will never get the
> >> update.
> > 
> > Just like all the other devices that require firmware. This driver is
> > no different from the others, and it is also not the only one that
> > needs the firmware only in special cases. Just make the firmware part
> > of your ramdisk, or build the driver as a module.
> 
> I wonder why then there is no general solution that does not force the
> driver to be built as a module.

Why would you need anything like that? Are you saying that even if you
put the firmware into your ramdisk, the driver still fails to find the
firmware if it's statically build? If so, then there is something else
wrong.

> If there is none, the documentation
> should mention that somehow (sorry if it does, I missed it). Actually a
> solution like the one implemented in the driver you mentioned could be
> used by any driver that can wait to be updated when the system is
> running.
> 
> > Are these firmwares available linux-firmware (or are the going to be)?
> > https://git.kernel.org/?p=linux/kernel/git/firmware/linux-firmware.git
> > 
> > thanks,
> > 
> The firmware (at least for the tps6598x) can be tailored with a TI
> specific tool and it depends on the use case, so I suppose making it
> public does not make much sense.

Okay.

thanks,

-- 
heikki