diff mbox series

[net-next,v2,05/12] mlxsw: core_linecards: Expose HW revision and INI version

Message ID 20220719064847.3688226-6-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series mlxsw: Implement dev info and dev flash for line cards | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 3 maintainers not CCed: linux-doc@vger.kernel.org corbet@lwn.net jiri@nvidia.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 86 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko July 19, 2022, 6:48 a.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

Implement info_get() to expose HW revision of a linecard and loaded INI
version.

Example:

$ devlink dev info auxiliary/mlxsw_core.lc.0
auxiliary/mlxsw_core.lc.0:
  versions:
      fixed:
        hw.revision 0
      running:
        ini.version 4

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 Documentation/networking/devlink/mlxsw.rst    | 18 ++++++++++++
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  4 +++
 .../mellanox/mlxsw/core_linecard_dev.c        | 11 ++++++++
 .../ethernet/mellanox/mlxsw/core_linecards.c  | 28 +++++++++++++++++++
 4 files changed, 61 insertions(+)

Comments

Ido Schimmel July 20, 2022, 8:56 a.m. UTC | #1 
On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
> +				    struct devlink_info_req *req,
> +				    struct netlink_ext_ack *extack)
> +{
> +	char buf[32];
> +	int err;
> +
> +	mutex_lock(&linecard->lock);
> +	if (WARN_ON(!linecard->provisioned)) {
> +		err = 0;

Why not:

err = -EINVAL;

?

> +		goto unlock;
> +	}
> +
> +	sprintf(buf, "%d", linecard->hw_revision);
> +	err = devlink_info_version_fixed_put(req, "hw.revision", buf);
> +	if (err)
> +		goto unlock;
> +
> +	sprintf(buf, "%d", linecard->ini_version);
> +	err = devlink_info_version_running_put(req, "ini.version", buf);
> +	if (err)
> +		goto unlock;
> +
> +unlock:
> +	mutex_unlock(&linecard->lock);
> +	return err;
> +}
> +
>  static int
>  mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
>  			     u16 hw_revision, u16 ini_version)
> -- 
> 2.35.3
>
Jiri Pirko July 20, 2022, 12:27 p.m. UTC | #2 
Wed, Jul 20, 2022 at 10:56:35AM CEST, idosch@nvidia.com wrote:
>On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
>> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
>> +				    struct devlink_info_req *req,
>> +				    struct netlink_ext_ack *extack)
>> +{
>> +	char buf[32];
>> +	int err;
>> +
>> +	mutex_lock(&linecard->lock);
>> +	if (WARN_ON(!linecard->provisioned)) {
>> +		err = 0;
>
>Why not:
>
>err = -EINVAL;
>
>?

Well, a) this should not happen. No need to push error to the user for
this as the rest of the info message is still fine.


>
>> +		goto unlock;
>> +	}
>> +
>> +	sprintf(buf, "%d", linecard->hw_revision);
>> +	err = devlink_info_version_fixed_put(req, "hw.revision", buf);
>> +	if (err)
>> +		goto unlock;
>> +
>> +	sprintf(buf, "%d", linecard->ini_version);
>> +	err = devlink_info_version_running_put(req, "ini.version", buf);
>> +	if (err)
>> +		goto unlock;
>> +
>> +unlock:
>> +	mutex_unlock(&linecard->lock);
>> +	return err;
>> +}
>> +
>>  static int
>>  mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
>>  			     u16 hw_revision, u16 ini_version)
>> -- 
>> 2.35.3
>>
Ido Schimmel July 20, 2022, 12:43 p.m. UTC | #3 
On Wed, Jul 20, 2022 at 02:27:40PM +0200, Jiri Pirko wrote:
> Wed, Jul 20, 2022 at 10:56:35AM CEST, idosch@nvidia.com wrote:
> >On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
> >> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
> >> +				    struct devlink_info_req *req,
> >> +				    struct netlink_ext_ack *extack)
> >> +{
> >> +	char buf[32];
> >> +	int err;
> >> +
> >> +	mutex_lock(&linecard->lock);
> >> +	if (WARN_ON(!linecard->provisioned)) {
> >> +		err = 0;
> >
> >Why not:
> >
> >err = -EINVAL;
> >
> >?
> 
> Well, a) this should not happen. No need to push error to the user for
> this as the rest of the info message is still fine.

Not sure what you mean by "the rest of the info message is still fine".
Which info message? If the line card is not provisioned, then it
shouldn't even have a devlink instance and it will not appear in
"devlink dev info" dump.

I still do not understand why this error is severe enough to print a
WARNING to the kernel log, but not emit an error code to user space.

> 
> 
> >
> >> +		goto unlock;
> >> +	}
> >> +
> >> +	sprintf(buf, "%d", linecard->hw_revision);
> >> +	err = devlink_info_version_fixed_put(req, "hw.revision", buf);
> >> +	if (err)
> >> +		goto unlock;
> >> +
> >> +	sprintf(buf, "%d", linecard->ini_version);
> >> +	err = devlink_info_version_running_put(req, "ini.version", buf);
> >> +	if (err)
> >> +		goto unlock;
> >> +
> >> +unlock:
> >> +	mutex_unlock(&linecard->lock);
> >> +	return err;
> >> +}
> >> +
> >>  static int
> >>  mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
> >>  			     u16 hw_revision, u16 ini_version)
> >> -- 
> >> 2.35.3
> >>
Ido Schimmel July 20, 2022, 12:50 p.m. UTC | #4 
On Wed, Jul 20, 2022 at 03:43:17PM +0300, Ido Schimmel wrote:
> On Wed, Jul 20, 2022 at 02:27:40PM +0200, Jiri Pirko wrote:
> > Wed, Jul 20, 2022 at 10:56:35AM CEST, idosch@nvidia.com wrote:
> > >On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
> > >> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
> > >> +				    struct devlink_info_req *req,
> > >> +				    struct netlink_ext_ack *extack)
> > >> +{
> > >> +	char buf[32];
> > >> +	int err;
> > >> +
> > >> +	mutex_lock(&linecard->lock);
> > >> +	if (WARN_ON(!linecard->provisioned)) {
> > >> +		err = 0;
> > >
> > >Why not:
> > >
> > >err = -EINVAL;
> > >
> > >?
> > 
> > Well, a) this should not happen. No need to push error to the user for
> > this as the rest of the info message is still fine.
> 
> Not sure what you mean by "the rest of the info message is still fine".
> Which info message? If the line card is not provisioned, then it
> shouldn't even have a devlink instance and it will not appear in
> "devlink dev info" dump.

How about returning '-EOPNOTSUPP'? Looks like devlink will skip it in a
dump
Jiri Pirko July 20, 2022, 2:58 p.m. UTC | #5 
Wed, Jul 20, 2022 at 02:50:09PM CEST, idosch@nvidia.com wrote:
>On Wed, Jul 20, 2022 at 03:43:17PM +0300, Ido Schimmel wrote:
>> On Wed, Jul 20, 2022 at 02:27:40PM +0200, Jiri Pirko wrote:
>> > Wed, Jul 20, 2022 at 10:56:35AM CEST, idosch@nvidia.com wrote:
>> > >On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
>> > >> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
>> > >> +				    struct devlink_info_req *req,
>> > >> +				    struct netlink_ext_ack *extack)
>> > >> +{
>> > >> +	char buf[32];
>> > >> +	int err;
>> > >> +
>> > >> +	mutex_lock(&linecard->lock);
>> > >> +	if (WARN_ON(!linecard->provisioned)) {
>> > >> +		err = 0;
>> > >
>> > >Why not:
>> > >
>> > >err = -EINVAL;
>> > >
>> > >?
>> > 
>> > Well, a) this should not happen. No need to push error to the user for
>> > this as the rest of the info message is still fine.
>> 
>> Not sure what you mean by "the rest of the info message is still fine".
>> Which info message? If the line card is not provisioned, then it
>> shouldn't even have a devlink instance and it will not appear in
>> "devlink dev info" dump.
>
>How about returning '-EOPNOTSUPP'? Looks like devlink will skip it in a
>dump

Okay.
Jiri Pirko July 20, 2022, 2:59 p.m. UTC | #6 
Wed, Jul 20, 2022 at 02:43:17PM CEST, idosch@nvidia.com wrote:
>On Wed, Jul 20, 2022 at 02:27:40PM +0200, Jiri Pirko wrote:
>> Wed, Jul 20, 2022 at 10:56:35AM CEST, idosch@nvidia.com wrote:
>> >On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
>> >> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
>> >> +				    struct devlink_info_req *req,
>> >> +				    struct netlink_ext_ack *extack)
>> >> +{
>> >> +	char buf[32];
>> >> +	int err;
>> >> +
>> >> +	mutex_lock(&linecard->lock);
>> >> +	if (WARN_ON(!linecard->provisioned)) {
>> >> +		err = 0;
>> >
>> >Why not:
>> >
>> >err = -EINVAL;
>> >
>> >?
>> 
>> Well, a) this should not happen. No need to push error to the user for
>> this as the rest of the info message is still fine.
>
>Not sure what you mean by "the rest of the info message is still fine".
>Which info message? If the line card is not provisioned, then it
>shouldn't even have a devlink instance and it will not appear in
>"devlink dev info" dump.
>
>I still do not understand why this error is severe enough to print a
>WARNING to the kernel log, but not emit an error code to user space.

As I wrote, WARN_ON was a leftover.

>
>> 
>> 
>> >
>> >> +		goto unlock;
>> >> +	}
>> >> +
>> >> +	sprintf(buf, "%d", linecard->hw_revision);
>> >> +	err = devlink_info_version_fixed_put(req, "hw.revision", buf);
>> >> +	if (err)
>> >> +		goto unlock;
>> >> +
>> >> +	sprintf(buf, "%d", linecard->ini_version);
>> >> +	err = devlink_info_version_running_put(req, "ini.version", buf);
>> >> +	if (err)
>> >> +		goto unlock;
>> >> +
>> >> +unlock:
>> >> +	mutex_unlock(&linecard->lock);
>> >> +	return err;
>> >> +}
>> >> +
>> >>  static int
>> >>  mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
>> >>  			     u16 hw_revision, u16 ini_version)
>> >> -- 
>> >> 2.35.3
>> >>
Ido Schimmel July 20, 2022, 3:01 p.m. UTC | #7 
On Wed, Jul 20, 2022 at 04:59:03PM +0200, Jiri Pirko wrote:
> Wed, Jul 20, 2022 at 02:43:17PM CEST, idosch@nvidia.com wrote:
> >On Wed, Jul 20, 2022 at 02:27:40PM +0200, Jiri Pirko wrote:
> >> Wed, Jul 20, 2022 at 10:56:35AM CEST, idosch@nvidia.com wrote:
> >> >On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
> >> >> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
> >> >> +				    struct devlink_info_req *req,
> >> >> +				    struct netlink_ext_ack *extack)
> >> >> +{
> >> >> +	char buf[32];
> >> >> +	int err;
> >> >> +
> >> >> +	mutex_lock(&linecard->lock);
> >> >> +	if (WARN_ON(!linecard->provisioned)) {
> >> >> +		err = 0;
> >> >
> >> >Why not:
> >> >
> >> >err = -EINVAL;
> >> >
> >> >?
> >> 
> >> Well, a) this should not happen. No need to push error to the user for
> >> this as the rest of the info message is still fine.
> >
> >Not sure what you mean by "the rest of the info message is still fine".
> >Which info message? If the line card is not provisioned, then it
> >shouldn't even have a devlink instance and it will not appear in
> >"devlink dev info" dump.
> >
> >I still do not understand why this error is severe enough to print a
> >WARNING to the kernel log, but not emit an error code to user space.
> 
> As I wrote, WARN_ON was a leftover.

It was a leftover in patch #10 where you checked '!linecard->ready'.
Here I think it's actually correct because it shouldn't happen unless
I'm missing something
Jiri Pirko July 20, 2022, 3:09 p.m. UTC | #8 
Wed, Jul 20, 2022 at 05:01:12PM CEST, idosch@nvidia.com wrote:
>On Wed, Jul 20, 2022 at 04:59:03PM +0200, Jiri Pirko wrote:
>> Wed, Jul 20, 2022 at 02:43:17PM CEST, idosch@nvidia.com wrote:
>> >On Wed, Jul 20, 2022 at 02:27:40PM +0200, Jiri Pirko wrote:
>> >> Wed, Jul 20, 2022 at 10:56:35AM CEST, idosch@nvidia.com wrote:
>> >> >On Tue, Jul 19, 2022 at 08:48:40AM +0200, Jiri Pirko wrote:
>> >> >> +int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
>> >> >> +				    struct devlink_info_req *req,
>> >> >> +				    struct netlink_ext_ack *extack)
>> >> >> +{
>> >> >> +	char buf[32];
>> >> >> +	int err;
>> >> >> +
>> >> >> +	mutex_lock(&linecard->lock);
>> >> >> +	if (WARN_ON(!linecard->provisioned)) {
>> >> >> +		err = 0;
>> >> >
>> >> >Why not:
>> >> >
>> >> >err = -EINVAL;
>> >> >
>> >> >?
>> >> 
>> >> Well, a) this should not happen. No need to push error to the user for
>> >> this as the rest of the info message is still fine.
>> >
>> >Not sure what you mean by "the rest of the info message is still fine".
>> >Which info message? If the line card is not provisioned, then it
>> >shouldn't even have a devlink instance and it will not appear in
>> >"devlink dev info" dump.
>> >
>> >I still do not understand why this error is severe enough to print a
>> >WARNING to the kernel log, but not emit an error code to user space.
>> 
>> As I wrote, WARN_ON was a leftover.
>
>It was a leftover in patch #10 where you checked '!linecard->ready'.
>Here I think it's actually correct because it shouldn't happen unless
>I'm missing something

Correct, I confused myself :)
diff mbox series

Patch

diff --git a/Documentation/networking/devlink/mlxsw.rst b/Documentation/networking/devlink/mlxsw.rst
index cf857cb4ba8f..aededcf68df4 100644
--- a/Documentation/networking/devlink/mlxsw.rst
+++ b/Documentation/networking/devlink/mlxsw.rst
@@ -58,6 +58,24 @@  The ``mlxsw`` driver reports the following versions
      - running
      - Three digit firmware version
 
+Line card auxiliary device info versions
+========================================
+
+The ``mlxsw`` driver reports the following versions for line card auxiliary device
+
+.. list-table:: devlink info versions implemented
+   :widths: 5 5 90
+
+   * - Name
+     - Type
+     - Description
+   * - ``hw.revision``
+     - fixed
+     - The hardware revision for this line card
+   * - ``ini.version``
+     - running
+     - Version of line card INI loaded
+
 Driver-specific Traps
 =====================
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index b22db13fa547..87c58b512536 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -599,6 +599,10 @@  mlxsw_linecard_get(struct mlxsw_linecards *linecards, u8 slot_index)
 	return &linecards->linecards[slot_index - 1];
 }
 
+int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
+				    struct devlink_info_req *req,
+				    struct netlink_ext_ack *extack);
+
 int mlxsw_linecards_init(struct mlxsw_core *mlxsw_core,
 			 const struct mlxsw_bus_info *bus_info);
 void mlxsw_linecards_fini(struct mlxsw_core *mlxsw_core);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c
index f41662936a2b..d0ecefee587b 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecard_dev.c
@@ -97,7 +97,18 @@  void mlxsw_linecard_bdev_del(struct mlxsw_linecard *linecard)
 	linecard->bdev = NULL;
 }
 
+static int mlxsw_linecard_dev_devlink_info_get(struct devlink *devlink,
+					       struct devlink_info_req *req,
+					       struct netlink_ext_ack *extack)
+{
+	struct mlxsw_linecard_dev *linecard_dev = devlink_priv(devlink);
+	struct mlxsw_linecard *linecard = linecard_dev->linecard;
+
+	return mlxsw_linecard_devlink_info_get(linecard, req, extack);
+}
+
 static const struct devlink_ops mlxsw_linecard_dev_devlink_ops = {
+	.info_get			= mlxsw_linecard_dev_devlink_info_get,
 };
 
 static int mlxsw_linecard_bdev_probe(struct auxiliary_device *adev,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
index 43696d8badca..c427e07b25dd 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core_linecards.c
@@ -226,6 +226,34 @@  void mlxsw_linecards_event_ops_unregister(struct mlxsw_core *mlxsw_core,
 }
 EXPORT_SYMBOL(mlxsw_linecards_event_ops_unregister);
 
+int mlxsw_linecard_devlink_info_get(struct mlxsw_linecard *linecard,
+				    struct devlink_info_req *req,
+				    struct netlink_ext_ack *extack)
+{
+	char buf[32];
+	int err;
+
+	mutex_lock(&linecard->lock);
+	if (WARN_ON(!linecard->provisioned)) {
+		err = 0;
+		goto unlock;
+	}
+
+	sprintf(buf, "%d", linecard->hw_revision);
+	err = devlink_info_version_fixed_put(req, "hw.revision", buf);
+	if (err)
+		goto unlock;
+
+	sprintf(buf, "%d", linecard->ini_version);
+	err = devlink_info_version_running_put(req, "ini.version", buf);
+	if (err)
+		goto unlock;
+
+unlock:
+	mutex_unlock(&linecard->lock);
+	return err;
+}
+
 static int
 mlxsw_linecard_provision_set(struct mlxsw_linecard *linecard, u8 card_type,
 			     u16 hw_revision, u16 ini_version)