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

  The firmware request process is device agnostic and can be used for
other parts.

A probe deferring mechanism has been added in order to account for cases
where the file system where the firmware resides is still not available
when the probe function is triggered and no firmware is found.

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/typec/tipd/core.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)
  

Hi Javier,

On Thu, Dec 07, 2023 at 12:51:07PM +0100, Javier Carrasco wrote:
> The firmware request process is device agnostic and can be used for
> other parts.
> 
> A probe deferring mechanism has been added in order to account for cases
> where the file system where the firmware resides is still not available
> when the probe function is triggered and no firmware is found.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  drivers/usb/typec/tipd/core.c | 36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index f0c4cd571a37..165a1391dc72 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -873,6 +873,31 @@ tps6598x_register_port(struct tps6598x *tps, struct fwnode_handle *fwnode)
>  	return 0;
>  }
>  
> +static int tps_request_firmware(struct tps6598x *tps, const struct firmware **fw)
> +{
> +	const char *firmware_name;
> +	int ret;
> +
> +	ret = device_property_read_string(tps->dev, "firmware-name",
> +					  &firmware_name);
> +	if (ret)
> +		return ret;
> +
> +	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);

> +	}
> +
> +	if ((*fw)->size == 0) {
> +		release_firmware(*fw);
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static int
>  tps25750_write_firmware(struct tps6598x *tps,
>  			u8 bpms_addr, const u8 *data, size_t len)
> @@ -961,16 +986,9 @@ static int tps25750_start_patch_burst_mode(struct tps6598x *tps)
>  	if (ret)
>  		return ret;
>  
> -	ret = request_firmware(&fw, firmware_name, tps->dev);
> -	if (ret) {
> -		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
> +	ret = tps_request_firmware(tps, &fw);
> +	if (ret)
>  		return ret;
> -	}
> -
> -	if (fw->size == 0) {
> -		ret = -EINVAL;
> -		goto release_fw;
> -	}
>  
>  	ret = of_property_match_string(np, "reg-names", "patch-address");
>  	if (ret < 0) {
> 
> -- 
> 2.39.2

thanks,
  

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.

Note that we are only triggering the update if the device is in patch
mode, so a firmware will be expected for the device to run and reach the
app mode.
In that case deferring the probe and keeping on trying to make the
update makes sense because otherwise the device will not be able to
offer its functionality. If the device is not in patch mode, no update
will be triggered and the firmware will not be requested, so there will
not be any unnecessary probe deferring.

I see that the driver you mentioned checks if the system_state is still
not SYSTEM_RUNNING to defer the probe.
I have not tested if something like that would be possible in this case,
but giving up on the first attempt if the firmware is not found makes
the assumption that the filesystem where the fw resides will always be
ready when the probe function is called, which in my particular case is
a wrong assumption.

If the firmware was updated at any point during normal operation, the
assumption would be definitely right, but maybe not while booting.

Thank you and best regards,
Javier Carrasco
  

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.

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,
  

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. 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.

Best regards,
Javier Carrasco
  

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,
  

On 13.12.23 16:15, Heikki Krogerus wrote:
> On Tue, Dec 12, 2023 at 03:41:35PM +0100, Javier Carrasco wrote:
>> 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.
> 
The firmware is always found unless the file system is still not ready,
which is the case on the system I am working on. If the driver is built
as a module, the issue is gone as expected.

My point was that there is no limitation to have the driver built-in and
no documentation to reference, so anyone could stumble on the same
issue. And as you said, this driver is not special in that sense, so
other drivers might be facing the same eventuality.
Am I missing any existing documentation for the fact that the firmware
must be put into the ramdisk or the driver must be built as a module? Or
is it only based on common sense?

Anyway the next version will not have any probe deferring and only
return an error if the firmware is not available.

Thanks and best regards,
Javier Carrasco