From: Jacob Keller <[email protected]>
[ Upstream commit a8f89fa27773a8c96fd09fb4e2f4892d794f21f6 ]
The devlink dev info command reports version information about the
device and firmware running on the board. This includes the "board.id"
field which is supposed to represent an identifier of the board design.
The ice driver uses the Product Board Assembly identifier for this.
In some cases, the PBA is not present in the NVM. If this happens,
devlink dev info will fail with an error. Instead, modify the
ice_info_pba function to just exit without filling in the context
buffer. This will cause the board.id field to be skipped. Log a dev_dbg
message in case someone wants to confirm why board.id is not showing up
for them.
Fixes: e961b679fb0b ("ice: add board identifier info to devlink .info_get")
Signed-off-by: Jacob Keller <[email protected]>
Tested-by: Tony Brelinski <[email protected]>
Signed-off-by: Tony Nguyen <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
drivers/net/ethernet/intel/ice/ice_devlink.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index 511da59bd6f2..f18ce43b7e74 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -23,7 +23,9 @@ static int ice_info_pba(struct ice_pf *pf, char *buf, size_t len)
status = ice_read_pba_string(hw, (u8 *)buf, len);
if (status)
- return -EIO;
+ /* We failed to locate the PBA, so just skip this entry */
+ dev_dbg(ice_pf_to_dev(pf), "Failed to read Product Board Assembly string, status %s\n",
+ ice_stat_str(status));
return 0;
}
--
2.30.2
Hi!
> > The devlink dev info command reports version information about the
> > device and firmware running on the board. This includes the "board.id"
> > field which is supposed to represent an identifier of the board design.
> > The ice driver uses the Product Board Assembly identifier for this.
> >
> > In some cases, the PBA is not present in the NVM. If this happens,
> > devlink dev info will fail with an error. Instead, modify the
> > ice_info_pba function to just exit without filling in the context
> > buffer. This will cause the board.id field to be skipped. Log a dev_dbg
> > message in case someone wants to confirm why board.id is not showing up
> > for them.
>
> Will it cause field to be skipped? I believe buffer will not be
> initialized which will result in some confusion...
IOW I believe this is good idea.
Signed-off-by: Pavel Machek (CIP) <[email protected]>
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index f18ce43b7e74..7034704b1b50 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -22,10 +22,12 @@ static int ice_info_pba(struct ice_pf *pf, char *buf, size_t len)
enum ice_status status;
status = ice_read_pba_string(hw, (u8 *)buf, len);
- if (status)
+ if (status) {
+ *buf = 0;
/* We failed to locate the PBA, so just skip this entry */
dev_dbg(ice_pf_to_dev(pf), "Failed to read Product Board Assembly string, status %s\n",
ice_stat_str(status));
+ }
return 0;
}
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
On 9/1/2021 1:10 PM, Pavel Machek wrote:
> Hi!
>
>>> The devlink dev info command reports version information about the
>>> device and firmware running on the board. This includes the "board.id"
>>> field which is supposed to represent an identifier of the board design.
>>> The ice driver uses the Product Board Assembly identifier for this.
>>>
>>> In some cases, the PBA is not present in the NVM. If this happens,
>>> devlink dev info will fail with an error. Instead, modify the
>>> ice_info_pba function to just exit without filling in the context
>>> buffer. This will cause the board.id field to be skipped. Log a dev_dbg
>>> message in case someone wants to confirm why board.id is not showing up
>>> for them.
>>
>> Will it cause field to be skipped? I believe buffer will not be
>> initialized which will result in some confusion...
>
> IOW I believe this is good idea.
>
It's not necessary, but I agree its not obvious without the full
context. The caller of ice_info_pba memsets the buffer before calling
each info reporter. Its already a known semantics that leaving the
buffer alone will skip the entry.
See the code below for what we do.
> memset(ctx->buf, 0, sizeof(ctx->buf));
>
> err = ice_devlink_versions[i].getter(pf, ctx);
> if (err) {
> NL_SET_ERR_MSG_MOD(extack, "Unable to obtain version info");
> goto out_free_ctx;
> }
>
> /* If the default getter doesn't report a version, use the
> * fallback function. This is primarily useful in the case of
> * "stored" versions that want to report the same value as the
> * running version in the normal case of no pending update.
> */
> if (ctx->buf[0] == '\0' && ice_devlink_versions[i].fallback) {
> err = ice_devlink_versions[i].fallback(pf, ctx);
> if (err) {
> NL_SET_ERR_MSG_MOD(extack, "Unable to obtain version info");
> goto out_free_ctx;
> }
> }
> /* Do not report missing versions */
> if (ctx->buf[0] == '\0')
> continue;
>
We memset the buffer, call the getter, and if that doesn't modify the
buffer, we call the fallack, and then check again if its still empty.
Because we memset each time, we don't need to assign *buf = 0.
I guess its more clear that we're doing the correct thing here, but
these functions are build-for-purpose to use as pointers in this API and
aren't public, so I think it is fine to leave it as is.
Thanks,
Jake
> Signed-off-by: Pavel Machek (CIP) <[email protected]>
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index f18ce43b7e74..7034704b1b50 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -22,10 +22,12 @@ static int ice_info_pba(struct ice_pf *pf, char *buf, size_t len)
> enum ice_status status;
>
> status = ice_read_pba_string(hw, (u8 *)buf, len);
> - if (status)
> + if (status) {
> + *buf = 0;
> /* We failed to locate the PBA, so just skip this entry */
> dev_dbg(ice_pf_to_dev(pf), "Failed to read Product Board Assembly string, status %s\n",
> ice_stat_str(status));
> + }
>
> return 0;
> }
>
Hi!
> From: Jacob Keller <[email protected]>
>
> [ Upstream commit a8f89fa27773a8c96fd09fb4e2f4892d794f21f6 ]
>
> The devlink dev info command reports version information about the
> device and firmware running on the board. This includes the "board.id"
> field which is supposed to represent an identifier of the board design.
> The ice driver uses the Product Board Assembly identifier for this.
>
> In some cases, the PBA is not present in the NVM. If this happens,
> devlink dev info will fail with an error. Instead, modify the
> ice_info_pba function to just exit without filling in the context
> buffer. This will cause the board.id field to be skipped. Log a dev_dbg
> message in case someone wants to confirm why board.id is not showing up
> for them.
Will it cause field to be skipped? I believe buffer will not be
initialized which will result in some confusion...
Best regards,
Pavel
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -23,7 +23,9 @@ static int ice_info_pba(struct ice_pf *pf, char *buf, size_t len)
>
> status = ice_read_pba_string(hw, (u8 *)buf, len);
> if (status)
> - return -EIO;
> + /* We failed to locate the PBA, so just skip this entry */
> + dev_dbg(ice_pf_to_dev(pf), "Failed to read Product Board Assembly string, status %s\n",
> + ice_stat_str(status));
>
> return 0;
> }
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hi!
> >>> The devlink dev info command reports version information about the
> >>> device and firmware running on the board. This includes the "board.id"
> >>> field which is supposed to represent an identifier of the board design.
> >>> The ice driver uses the Product Board Assembly identifier for this.
> >>>
> >>> In some cases, the PBA is not present in the NVM. If this happens,
> >>> devlink dev info will fail with an error. Instead, modify the
> >>> ice_info_pba function to just exit without filling in the context
> >>> buffer. This will cause the board.id field to be skipped. Log a dev_dbg
> >>> message in case someone wants to confirm why board.id is not showing up
> >>> for them.
> >>
> >> Will it cause field to be skipped? I believe buffer will not be
> >> initialized which will result in some confusion...
> >
> > IOW I believe this is good idea.
>
> It's not necessary, but I agree its not obvious without the full
> context. The caller of ice_info_pba memsets the buffer before calling
> each info reporter. Its already a known semantics that leaving the
> buffer alone will skip the entry.
>
> See the code below for what we do.
>
> > memset(ctx->buf, 0, sizeof(ctx->buf));
> >
> > err = ice_devlink_versions[i].getter(pf, ctx);
> > if (err) {
> > NL_SET_ERR_MSG_MOD(extack, "Unable to obtain version info");
> > goto out_free_ctx;
> > }
That memset is not present in 5.10 I was reviewing. I agree that
backporting the memset to 5.10 is better then my patch.
> We memset the buffer, call the getter, and if that doesn't modify the
> buffer, we call the fallack, and then check again if its still empty.
>
> Because we memset each time, we don't need to assign *buf = 0.
>
> I guess its more clear that we're doing the correct thing here, but
> these functions are build-for-purpose to use as pointers in this API and
> aren't public, so I think it is fine to leave it as is.
Yes, code is okay in mainline, but the memset is not present in
5.10-stable.
Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany