| Message ID | 20231207-tps6598x_update-v1-2-dc21b5301d91@wolfvision.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tipd: add patch update support for tps6598x | expand |
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
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; + } + + 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) {
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(-)