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