| Message ID | 20241119-v6-10-topic-touchscreen-axiom-v1-2-6124925b9718@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | Input: Add support for TouchNetix aXiom touchscreen | expand |
On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote: > It's no error if a driver indicates that the firmware is already > up-to-date and the update can be skipped. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/base/firmware_loader/sysfs_upload.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c > index b3cbe5b156e3..44f3d8fa5e64 100644 > --- a/drivers/base/firmware_loader/sysfs_upload.c > +++ b/drivers/base/firmware_loader/sysfs_upload.c > @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work) > fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING); > ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size); > if (ret != FW_UPLOAD_ERR_NONE) { > + if (ret == FW_UPLOAD_ERR_SKIP) { > + dev_info(fw_dev, "firmware already up-to-date, skip update\n"); > + ret = FW_UPLOAD_ERR_NONE; > + } If you change the error-code from FW_UPLOAD_ERR_SKIP to FW_UPLOAD_ERR_NONE, then the "skip" string provided in the previous patch will never be seen. There are currently no other instances where an error code requires special-case modifications to the fw_upload code and I don't think it is necessary to add it here. The dev_info() message above can be provided by the device driver that is using this API. I think you can either: (1) allow "skip" to be treated as an error. The update didn't happen... -or- (2) The prepare function could detect the situation and set a flag in the same device driver. Your write function could set *written to the full data size and return without writing anything. Your poll_complete handler could also return FW_UPLOAD_ERR_NONE. Then you don't need to add FW_UPLOAD_ERR_SKIP at all. You would get the info message from the device driver and fw_upload would exit without an error. Thanks, - Russ > fw_upload_set_error(fwlp, ret); > goto putdev_exit; > } > > -- > 2.39.5 >
Hi, On 24-11-20, Russ Weight wrote: > On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote: > > It's no error if a driver indicates that the firmware is already > > up-to-date and the update can be skipped. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/base/firmware_loader/sysfs_upload.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c > > index b3cbe5b156e3..44f3d8fa5e64 100644 > > --- a/drivers/base/firmware_loader/sysfs_upload.c > > +++ b/drivers/base/firmware_loader/sysfs_upload.c > > @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work) > > fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING); > > ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size); > > if (ret != FW_UPLOAD_ERR_NONE) { > > + if (ret == FW_UPLOAD_ERR_SKIP) { > > + dev_info(fw_dev, "firmware already up-to-date, skip update\n"); > > + ret = FW_UPLOAD_ERR_NONE; > > + } > > If you change the error-code from FW_UPLOAD_ERR_SKIP to > FW_UPLOAD_ERR_NONE, then the "skip" string provided in the previous > patch will never be seen. There are currently no other instances where Do we really need to set it? As explained within the commit message, it's no error if FW_UPLOAD_ERR_SKIP is returned. The previous patch just added all pieces which may be required later on. > an error code requires special-case modifications to the fw_upload > code and I don't think it is necessary to add it here. Because at the moment no one is checking it except for the gb-beagleplay driver. This driver prints a dev_warn() string and returns a failure. Now the userspace needs some heuristic by parsing dmesg to check the reason. This is rather complex and very error prone as the sting can be changed in the future. Therefore I added the support to have a simple error code which can be returned by a driver. I'm open to return "skip" as error instead of casting it to none. Both is fine for me since both allow the userspace to easily check if the error is a 'real' error or if the fw-update was just skipped due to already-up-to-date. I wouldn't say that this is a special case, it is very common but no one is performing a fw-version check. Therefore I added this to the common code, to make it easier for driver devs. > The dev_info() message above can be provided by the device driver > that is using this API. > > I think you can either: > > (1) allow "skip" to be treated as an error. The update didn't happen... Please see above. > -or- > > (2) The prepare function could detect the situation and set > a flag in the same device driver. Your write function could > set *written to the full data size and return without writing > anything. Your poll_complete handler could also return > FW_UPLOAD_ERR_NONE. Then you don't need to add FW_UPLOAD_ERR_SKIP > at all. You would get the info message from the device driver > and fw_upload would exit without an error. Please see above. I don't think that this is special case and why making the life hard for driver devs instead of having a well known fw behaviour? Regards, Marco > > Thanks, > - Russ > > > fw_upload_set_error(fwlp, ret); > > goto putdev_exit; > > } > > > > -- > > 2.39.5 > > >
On Wed, Nov 20, 2024 at 06:30:37PM +0100, Marco Felsch wrote: > Hi, > > On 24-11-20, Russ Weight wrote: > > On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote: > > > It's no error if a driver indicates that the firmware is already > > > up-to-date and the update can be skipped. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > drivers/base/firmware_loader/sysfs_upload.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c > > > index b3cbe5b156e3..44f3d8fa5e64 100644 > > > --- a/drivers/base/firmware_loader/sysfs_upload.c > > > +++ b/drivers/base/firmware_loader/sysfs_upload.c > > > @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work) > > > fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING); > > > ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size); > > > if (ret != FW_UPLOAD_ERR_NONE) { > > > + if (ret == FW_UPLOAD_ERR_SKIP) { > > > + dev_info(fw_dev, "firmware already up-to-date, skip update\n"); > > > + ret = FW_UPLOAD_ERR_NONE; > > > + } > > > > If you change the error-code from FW_UPLOAD_ERR_SKIP to > > FW_UPLOAD_ERR_NONE, then the "skip" string provided in the previous > > patch will never be seen. There are currently no other instances where > > Do we really need to set it? As explained within the commit message, > it's no error if FW_UPLOAD_ERR_SKIP is returned. The previous patch just > added all pieces which may be required later on. > > > an error code requires special-case modifications to the fw_upload > > code and I don't think it is necessary to add it here. > > Because at the moment no one is checking it except for the gb-beagleplay > driver. This driver prints a dev_warn() string and returns a failure. > Now the userspace needs some heuristic by parsing dmesg to check the > reason. This is rather complex and very error prone as the sting can be > changed in the future. > > Therefore I added the support to have a simple error code which can be > returned by a driver. I'm open to return "skip" as error instead of > casting it to none. Both is fine for me since both allow the userspace > to easily check if the error is a 'real' error or if the fw-update was > just skipped due to already-up-to-date. Are you saying that you intend for the user-space code to see "skip"? Because in the current implementation, I don't think the user-space code would see "skip". If you ultimately return FW_UPLOAD_ERR_NONE, then cat'ing the error file should result in an empty file. > > I wouldn't say that this is a special case, it is very common but no one > is performing a fw-version check. Therefore I added this to the common > code, to make it easier for driver devs. By "special case" I meant to say that this is the first time this core code has had to know about any error codes other than FW_UPLOAD_ERR_NONE - and the first time that an error type alters the code flow. I understand that other drivers may also want to abort if the firmware being loaded is a duplicate. > > > The dev_info() message above can be provided by the device driver > > that is using this API. > > > > I think you can either: > > > > (1) allow "skip" to be treated as an error. The update didn't happen... > > Please see above. > > > -or- > > > > (2) The prepare function could detect the situation and set > > a flag in the same device driver. Your write function could > > set *written to the full data size and return without writing > > anything. Your poll_complete handler could also return > > FW_UPLOAD_ERR_NONE. Then you don't need to add FW_UPLOAD_ERR_SKIP > > at all. You would get the info message from the device driver > > and fw_upload would exit without an error. > > Please see above. I don't think that this is special case and why making > the life hard for driver devs instead of having a well known fw > behaviour? If you are not opposed to treating it as an error, then all you need to add are the error code and the string to go with it. Instead of FW_UPLOAD_ERR_SKIP -> "skip", how about FW_UPLOAD_ERR_DUPLICATE -> "duplicate_firmware"? Thanks, - Russ > > Regards, > Marco > > > > > Thanks, > > - Russ > > > > > fw_upload_set_error(fwlp, ret); > > > goto putdev_exit; > > > } > > > > > > -- > > > 2.39.5 > > > > >
On 24-11-20, Russ Weight wrote: > > On Wed, Nov 20, 2024 at 06:30:37PM +0100, Marco Felsch wrote: > > Hi, > > > > On 24-11-20, Russ Weight wrote: > > > On Tue, Nov 19, 2024 at 11:33:51PM +0100, Marco Felsch wrote: > > > > It's no error if a driver indicates that the firmware is already > > > > up-to-date and the update can be skipped. > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > --- > > > > drivers/base/firmware_loader/sysfs_upload.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c > > > > index b3cbe5b156e3..44f3d8fa5e64 100644 > > > > --- a/drivers/base/firmware_loader/sysfs_upload.c > > > > +++ b/drivers/base/firmware_loader/sysfs_upload.c > > > > @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work) > > > > fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING); > > > > ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size); > > > > if (ret != FW_UPLOAD_ERR_NONE) { > > > > + if (ret == FW_UPLOAD_ERR_SKIP) { > > > > + dev_info(fw_dev, "firmware already up-to-date, skip update\n"); > > > > + ret = FW_UPLOAD_ERR_NONE; > > > > + } > > > > > > If you change the error-code from FW_UPLOAD_ERR_SKIP to > > > FW_UPLOAD_ERR_NONE, then the "skip" string provided in the previous > > > patch will never be seen. There are currently no other instances where > > > > Do we really need to set it? As explained within the commit message, > > it's no error if FW_UPLOAD_ERR_SKIP is returned. The previous patch just > > added all pieces which may be required later on. > > > > > an error code requires special-case modifications to the fw_upload > > > code and I don't think it is necessary to add it here. > > > > Because at the moment no one is checking it except for the gb-beagleplay > > driver. This driver prints a dev_warn() string and returns a failure. > > Now the userspace needs some heuristic by parsing dmesg to check the > > reason. This is rather complex and very error prone as the sting can be > > changed in the future. > > > > Therefore I added the support to have a simple error code which can be > > returned by a driver. I'm open to return "skip" as error instead of > > casting it to none. Both is fine for me since both allow the userspace > > to easily check if the error is a 'real' error or if the fw-update was > > just skipped due to already-up-to-date. > > Are you saying that you intend for the user-space code to see "skip"? > Because in the current implementation, I don't think the user-space > code would see "skip". If you ultimately return FW_UPLOAD_ERR_NONE, > then cat'ing the error file should result in an empty file. I know. > > I wouldn't say that this is a special case, it is very common but no one > > is performing a fw-version check. Therefore I added this to the common > > code, to make it easier for driver devs. > > By "special case" I meant to say that this is the first time this > core code has had to know about any error codes other than > FW_UPLOAD_ERR_NONE - and the first time that an error type alters > the code flow. > > I understand that other drivers may also want to abort if the > firmware being loaded is a duplicate. :) > > > The dev_info() message above can be provided by the device driver > > > that is using this API. > > > > > > I think you can either: > > > > > > (1) allow "skip" to be treated as an error. The update didn't happen... > > > > Please see above. > > > > > -or- > > > > > > (2) The prepare function could detect the situation and set > > > a flag in the same device driver. Your write function could > > > set *written to the full data size and return without writing > > > anything. Your poll_complete handler could also return > > > FW_UPLOAD_ERR_NONE. Then you don't need to add FW_UPLOAD_ERR_SKIP > > > at all. You would get the info message from the device driver > > > and fw_upload would exit without an error. > > > > Please see above. I don't think that this is special case and why making > > the life hard for driver devs instead of having a well known fw > > behaviour? > > If you are not opposed to treating it as an error, then all you need > to add are the error code and the string to go with it. Yep this works for me. > Instead of FW_UPLOAD_ERR_SKIP -> "skip", how about > FW_UPLOAD_ERR_DUPLICATE -> "duplicate_firmware"? Fine by me :) I can use this if no one else comes up with a better idea for the string. Regards, Marco
diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c index b3cbe5b156e3..44f3d8fa5e64 100644 --- a/drivers/base/firmware_loader/sysfs_upload.c +++ b/drivers/base/firmware_loader/sysfs_upload.c @@ -174,6 +174,10 @@ static void fw_upload_main(struct work_struct *work) fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PREPARING); ret = fwlp->ops->prepare(fwl, fwlp->data, fwlp->remaining_size); if (ret != FW_UPLOAD_ERR_NONE) { + if (ret == FW_UPLOAD_ERR_SKIP) { + dev_info(fw_dev, "firmware already up-to-date, skip update\n"); + ret = FW_UPLOAD_ERR_NONE; + } fw_upload_set_error(fwlp, ret); goto putdev_exit; }
It's no error if a driver indicates that the firmware is already up-to-date and the update can be skipped. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/base/firmware_loader/sysfs_upload.c | 4 ++++ 1 file changed, 4 insertions(+)