[PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

It could be more than one instance of LED with the same name in the
modular systems. For example, "status" or "uid" LED can be located
on chassis and on each line card of modular system.
In order to avoid conflicts with duplicated names, append platform
device Id, which is unquie, to LED name after driver name.
Thus, for example, "status" LED on chassis is to be called, like it is
called now on non modular systems, on which platform device Id is not
specified: "mlxreg:status:green". While for the line cards LEDs it will
be called like: "mlxreg48:status:green", "mlxreg66:status:green",
etcetera.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
---
 drivers/leds/leds-mlxreg.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
index 82aea1cd0c12..53130a8656b1 100644
--- a/drivers/leds/leds-mlxreg.c
+++ b/drivers/leds/leds-mlxreg.c
@@ -228,8 +228,12 @@ static int mlxreg_led_config(struct mlxreg_led_priv_data *priv)
 			brightness = LED_OFF;
 			led_data->base_color = MLXREG_LED_GREEN_SOLID;
 		}
-		snprintf(led_data->led_cdev_name, sizeof(led_data->led_cdev_name),
-			 "mlxreg:%s", data->label);
+		if (priv->pdev->id > 0)
+			sprintf(led_data->led_cdev_name, "%s%d:%s", "mlxreg",
+				priv->pdev->id, data->label);
+		else
+			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
+				data->label);
 		led_cdev->name = led_data->led_cdev_name;
 		led_cdev->brightness = brightness;
 		led_cdev->max_brightness = LED_ON;
-- 
2.11.0

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Marek Behun]

On Tue,  6 Oct 2020 19:58:50 +0300
Vadim Pasternak <vadimp@nvidia.com> wrote:

> It could be more than one instance of LED with the same name in the
> modular systems. For example, "status" or "uid" LED can be located
> on chassis and on each line card of modular system.
> In order to avoid conflicts with duplicated names, append platform
> device Id, which is unquie, to LED name after driver name.
> Thus, for example, "status" LED on chassis is to be called, like it is
> called now on non modular systems, on which platform device Id is not
> specified: "mlxreg:status:green". While for the line cards LEDs it will
> be called like: "mlxreg48:status:green", "mlxreg66:status:green",
> etcetera.

:( what types of modules are these? Are they hotpluggable network
adapter or something like that? What should I imagine for
example mlxreg48 device to be?

Btw it would be nice if mlx-platform was converted to Device Tree API
instead of registering each device in a system by hand.

Marek

> 
> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> ---
>  drivers/leds/leds-mlxreg.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
> index 82aea1cd0c12..53130a8656b1 100644
> --- a/drivers/leds/leds-mlxreg.c
> +++ b/drivers/leds/leds-mlxreg.c
> @@ -228,8 +228,12 @@ static int mlxreg_led_config(struct mlxreg_led_priv_data *priv)
>  			brightness = LED_OFF;
>  			led_data->base_color = MLXREG_LED_GREEN_SOLID;
>  		}
> -		snprintf(led_data->led_cdev_name, sizeof(led_data->led_cdev_name),
> -			 "mlxreg:%s", data->label);
> +		if (priv->pdev->id > 0)
> +			sprintf(led_data->led_cdev_name, "%s%d:%s", "mlxreg",
> +				priv->pdev->id, data->label);
> +		else
> +			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
> +				data->label);
>  		led_cdev->name = led_data->led_cdev_name;
>  		led_cdev->brightness = brightness;
>  		led_cdev->max_brightness = LED_ON;

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Marek Behun <marek.behun@nic.cz>
> Sent: Wednesday, October 07, 2020 2:15 AM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: jacek.anaszewski@gmail.com; linux-leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> On Tue,  6 Oct 2020 19:58:50 +0300
> Vadim Pasternak <vadimp@nvidia.com> wrote:
> 
> > It could be more than one instance of LED with the same name in the
> > modular systems. For example, "status" or "uid" LED can be located on
> > chassis and on each line card of modular system.
> > In order to avoid conflicts with duplicated names, append platform
> > device Id, which is unquie, to LED name after driver name.
> > Thus, for example, "status" LED on chassis is to be called, like it is
> > called now on non modular systems, on which platform device Id is not
> > specified: "mlxreg:status:green". While for the line cards LEDs it
> > will be called like: "mlxreg48:status:green", "mlxreg66:status:green",
> > etcetera.
> 
> :( what types of modules are these? Are they hotpluggable network adapter or
> something like that? What should I imagine for example mlxreg48 device to
> be?

This is new modular systems which could be equipped with
the different types of replaceable line cards and management
board. The first system flavor will support the line cards
equipped with Lattice CPLD devices aimed for system and
ASIC control, one Nvidia FPGA, and Nvidia Mellanox gearboxes for the port control and with 16x100GbE QSFP28 ports.
The next line cards flavors are supposed to be equipped with 8x200Gbe QSFP28 ports, 4x400Gbe QSFP-DD ports and few
flavors of smart cards equipped with Nvidia ARM CPU.

System is equipped with management card and has eight
slots for line cards.
All these line cards are replicable.

The system is based on Nvidia Spectrum-3 ASIC.
The switch height is 4U and it fits standard rack size.

> 
> Btw it would be nice if mlx-platform was converted to Device Tree API instead
> of registering each device in a system by hand.
> 

mlx-platform activates tens other platform drivers.
So this is a huge change, which will require huge amount
of new definitions for DT API - actually if will be register
map description at bit granularity.

I can think about it for the future.

Which real benefits you see from such move?
Currently all our system are based on x86 arch.

Vadim.

> Marek
> 
> >
> > Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> > ---
> >  drivers/leds/leds-mlxreg.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
> > index 82aea1cd0c12..53130a8656b1 100644
> > --- a/drivers/leds/leds-mlxreg.c
> > +++ b/drivers/leds/leds-mlxreg.c
> > @@ -228,8 +228,12 @@ static int mlxreg_led_config(struct
> mlxreg_led_priv_data *priv)
> >  			brightness = LED_OFF;
> >  			led_data->base_color = MLXREG_LED_GREEN_SOLID;
> >  		}
> > -		snprintf(led_data->led_cdev_name, sizeof(led_data-
> >led_cdev_name),
> > -			 "mlxreg:%s", data->label);
> > +		if (priv->pdev->id > 0)
> > +			sprintf(led_data->led_cdev_name, "%s%d:%s",
> "mlxreg",
> > +				priv->pdev->id, data->label);
> > +		else
> > +			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
> > +				data->label);
> >  		led_cdev->name = led_data->led_cdev_name;
> >  		led_cdev->brightness = brightness;
> >  		led_cdev->max_brightness = LED_ON;

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Marek Behun]

On Wed, 7 Oct 2020 06:07:23 +0000
Vadim Pasternak <vadimp@nvidia.com> wrote:

> mlx-platform activates tens other platform drivers.
> So this is a huge change, which will require huge amount
> of new definitions for DT API - actually if will be register
> map description at bit granularity.
> 
> I can think about it for the future.
> 
> Which real benefits you see from such move?
> Currently all our system are based on x86 arch.
> 
> Vadim.

Okay, it seems that would be a huge change, so never mind.
I was asking because the LED core constructs LED labels itself if there
are specific properties in device tree.

Marek

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Tue 2020-10-06 19:58:50, Vadim Pasternak wrote:
> It could be more than one instance of LED with the same name in the
> modular systems. For example, "status" or "uid" LED can be located
> on chassis and on each line card of modular system.
> In order to avoid conflicts with duplicated names, append platform
> device Id, which is unquie, to LED name after driver name.
> Thus, for example, "status" LED on chassis is to be called, like it is
> called now on non modular systems, on which platform device Id is not
> specified: "mlxreg:status:green". While for the line cards LEDs it will
> be called like: "mlxreg48:status:green", "mlxreg66:status:green",
> etcetera.

No.

You really should not have mlxreg: in the LED label. It is useless.

Make it so that LEDs on main body are ":foo:bar", and LEDs on the
expansion card has something reasonable as the device part.

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Marek Behun]

On Wed, 7 Oct 2020 13:31:05 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Tue 2020-10-06 19:58:50, Vadim Pasternak wrote:
> > It could be more than one instance of LED with the same name in the
> > modular systems. For example, "status" or "uid" LED can be located
> > on chassis and on each line card of modular system.
> > In order to avoid conflicts with duplicated names, append platform
> > device Id, which is unquie, to LED name after driver name.
> > Thus, for example, "status" LED on chassis is to be called, like it is
> > called now on non modular systems, on which platform device Id is not
> > specified: "mlxreg:status:green". While for the line cards LEDs it will
> > be called like: "mlxreg48:status:green", "mlxreg66:status:green",
> > etcetera.
> 
> No.
> 
> You really should not have mlxreg: in the LED label. It is useless.
> 
> Make it so that LEDs on main body are ":foo:bar", and LEDs on the
> expansion card has something reasonable as the device part.
> 
> Best regards,
> 								Pavel

Moreover the LED core, if there are more LEDs with same color and
function, constructs labels in the form
  [device:]color:function-functionenumerator
so if we want your driver to align with other LED drivers, you should put
the enumerator at the end of the label
  green:status-48
  green:status-66
...

Pavel, the LED core does not put the ':' symbol at the beginning if
there is no devicename. The LED name is only "color:function". Should
this change?

Marek

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Marek Behun <marek.behun@nic.cz>
> Sent: Wednesday, October 07, 2020 3:21 PM
> To: Pavel Machek <pavel@ucw.cz>
> Cc: Vadim Pasternak <vadimp@nvidia.com>; jacek.anaszewski@gmail.com;
> linux-leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> On Wed, 7 Oct 2020 13:31:05 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > On Tue 2020-10-06 19:58:50, Vadim Pasternak wrote:
> > > It could be more than one instance of LED with the same name in the
> > > modular systems. For example, "status" or "uid" LED can be located
> > > on chassis and on each line card of modular system.
> > > In order to avoid conflicts with duplicated names, append platform
> > > device Id, which is unquie, to LED name after driver name.
> > > Thus, for example, "status" LED on chassis is to be called, like it
> > > is called now on non modular systems, on which platform device Id is
> > > not
> > > specified: "mlxreg:status:green". While for the line cards LEDs it
> > > will be called like: "mlxreg48:status:green",
> > > "mlxreg66:status:green", etcetera.
> >
> > No.
> >
> > You really should not have mlxreg: in the LED label. It is useless.

'mlxreg' is device name, which could be CPLD or FPGA.
It should be in label.

There also few other reasons for that.

This name is used in thousands system in the field and
customers use it in their application.

We used to provide our ASIC and CPLD or FPGA logic
(Verilog) to ODM vendors, which build their own switch
on top of it and use our drivers. So, the can implement
additional LED on their switches, not controlled by our
drivers and device name 'mlxreg' allows to distinct between
LED objects.

Actually name like 'mlxreg48', 'mlxreg56' are yet another
'mlxreg' devices with appended physical bus Id.

This is not actually 'functionenumerator'.

> >
> > Make it so that LEDs on main body are ":foo:bar", and LEDs on the
> > expansion card has something reasonable as the device part.
> >
> > Best regards,
> > 								Pavel
> 
> Moreover the LED core, if there are more LEDs with same color and function,
> constructs labels in the form
>   [device:]color:function-functionenumerator
> so if we want your driver to align with other LED drivers, you should put the
> enumerator at the end of the label
>   green:status-48
>   green:status-66
> ...
> 
> Pavel, the LED core does not put the ':' symbol at the beginning if there is no
> devicename. The LED name is only "color:function". Should this change?
> 
> Marek

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

Hi!

> > > > It could be more than one instance of LED with the same name in the
> > > > modular systems. For example, "status" or "uid" LED can be located
> > > > on chassis and on each line card of modular system.
> > > > In order to avoid conflicts with duplicated names, append platform
> > > > device Id, which is unquie, to LED name after driver name.
> > > > Thus, for example, "status" LED on chassis is to be called, like it
> > > > is called now on non modular systems, on which platform device Id is
> > > > not
> > > > specified: "mlxreg:status:green". While for the line cards LEDs it
> > > > will be called like: "mlxreg48:status:green",
> > > > "mlxreg66:status:green", etcetera.
> > >
> > > No.
> > >
> > > You really should not have mlxreg: in the LED label. It is useless.
> 
> 'mlxreg' is device name, which could be CPLD or FPGA.
> It should be in label.

No.

You can try to explain what "mlxreg" means, but...

You can get away with "eth0" as a device name. We should talk about
"switch0" I guess.

> There also few other reasons for that.
> 
> This name is used in thousands system in the field and
> customers use it in their application.

That may be reason not to change existing names.

> We used to provide our ASIC and CPLD or FPGA logic
> (Verilog) to ODM vendors, which build their own switch
> on top of it and use our drivers. So, the can implement
> additional LED on their switches, not controlled by our
> drivers and device name 'mlxreg' allows to distinct between
> LED objects.
> 
> Actually name like 'mlxreg48', 'mlxreg56' are yet another
> 'mlxreg' devices with appended physical bus Id.

But noone is currently using mlxreg123 in their applications, so that
part is not going in.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Pavel Machek <pavel@ucw.cz>
> Sent: Thursday, October 08, 2020 10:56 AM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: Marek Behun <marek.behun@nic.cz>; jacek.anaszewski@gmail.com; linux-
> leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> Hi!
> 
> > > > > It could be more than one instance of LED with the same name in
> > > > > the modular systems. For example, "status" or "uid" LED can be
> > > > > located on chassis and on each line card of modular system.
> > > > > In order to avoid conflicts with duplicated names, append
> > > > > platform device Id, which is unquie, to LED name after driver name.
> > > > > Thus, for example, "status" LED on chassis is to be called, like
> > > > > it is called now on non modular systems, on which platform
> > > > > device Id is not
> > > > > specified: "mlxreg:status:green". While for the line cards LEDs
> > > > > it will be called like: "mlxreg48:status:green",
> > > > > "mlxreg66:status:green", etcetera.
> > > >
> > > > No.
> > > >
> > > > You really should not have mlxreg: in the LED label. It is useless.
> >
> > 'mlxreg' is device name, which could be CPLD or FPGA.
> > It should be in label.
> 
> No.
> 
> You can try to explain what "mlxreg" means, but...
> 
> You can get away with "eth0" as a device name. We should talk about
> "switch0" I guess.
> 
> > There also few other reasons for that.
> >
> > This name is used in thousands system in the field and customers use
> > it in their application.
> 
> That may be reason not to change existing names.
> 
> > We used to provide our ASIC and CPLD or FPGA logic
> > (Verilog) to ODM vendors, which build their own switch on top of it
> > and use our drivers. So, the can implement additional LED on their
> > switches, not controlled by our drivers and device name 'mlxreg'
> > allows to distinct between LED objects.
> >
> > Actually name like 'mlxreg48', 'mlxreg56' are yet another 'mlxreg'
> > devices with appended physical bus Id.
> 
> But noone is currently using mlxreg123 in their applications, so that part is not
> going in.

Yes, this is true.
I could modify it as:

		if (priv->pdev->id > 0)
			sprintf(led_data->led_cdev_name, "%s%d:%s", "card",
				priv->pdev->id, data->label);
		else
			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
				data->label);

> 
> Best regards,
> 
> 									Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Marek Behun]

On Thu, 8 Oct 2020 08:47:45 +0000
Vadim Pasternak <vadimp@nvidia.com> wrote:

> > 
> > But noone is currently using mlxreg123 in their applications, so that part is not
> > going in.  
> 
> Yes, this is true.
> I could modify it as:
> 
> 		if (priv->pdev->id > 0)
> 			sprintf(led_data->led_cdev_name, "%s%d:%s", "card",
> 				priv->pdev->id, data->label);
> 		else
> 			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
> 				data->label);
> 

Vadim, the LED core constructs names in form
  device:color:function-enumerator
so if you must have number there, IMO it should be
  mlxreg:green:status-48
  mlxreg:green:status-56
  ...

Marek

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Marek Behun <marek.behun@nic.cz>
> Sent: Thursday, October 08, 2020 11:56 AM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: Pavel Machek <pavel@ucw.cz>; jacek.anaszewski@gmail.com; linux-
> leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> On Thu, 8 Oct 2020 08:47:45 +0000
> Vadim Pasternak <vadimp@nvidia.com> wrote:
> 
> > >
> > > But noone is currently using mlxreg123 in their applications, so
> > > that part is not going in.
> >
> > Yes, this is true.
> > I could modify it as:
> >
> > 		if (priv->pdev->id > 0)
> > 			sprintf(led_data->led_cdev_name, "%s%d:%s", "card",
> > 				priv->pdev->id, data->label);
> > 		else
> > 			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
> > 				data->label);
> >
> 
> Vadim, the LED core constructs names in form
>   device:color:function-enumerator
> so if you must have number there, IMO it should be
>   mlxreg:green:status-48
>   mlxreg:green:status-56
>   ...

But why you consider it as function enumerator?
For example card48, card56 are two different devices
of same type.
Both have 'status' LED.

> 
> Marek

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

Hi!

> > Vadim, the LED core constructs names in form
> >   device:color:function-enumerator
> > so if you must have number there, IMO it should be
> >   mlxreg:green:status-48
> >   mlxreg:green:status-56
> >   ...
> 
> But why you consider it as function enumerator?
> For example card48, card56 are two different devices
> of same type.
> Both have 'status' LED.

It would help if you could explain what "mlxreg" is.

And yes, if you have some kind of device with a status LED, then you
can put that into the first card. For example sda::status would be
accetpable. But cardXX is way too generic.

Perhaps you can explain what "card" is in this context? What is its
main function?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Pavel Machek <pavel@ucw.cz>
> Sent: Thursday, October 08, 2020 1:05 PM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: Marek Behun <marek.behun@nic.cz>; jacek.anaszewski@gmail.com; linux-
> leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> Hi!
> 
> > > Vadim, the LED core constructs names in form
> > >   device:color:function-enumerator
> > > so if you must have number there, IMO it should be
> > >   mlxreg:green:status-48
> > >   mlxreg:green:status-56
> > >   ...
> >
> > But why you consider it as function enumerator?
> > For example card48, card56 are two different devices of same type.
> > Both have 'status' LED.
> 
> It would help if you could explain what "mlxreg" is.

This is common name for CPLD or FPGA register map, shared between different
platform drivers, while CPLD or FPGA can be connected to CPU trough LPC, SPI or
I2C bus.


> 
> And yes, if you have some kind of device with a status LED, then you can put
> that into the first card. For example sda::status would be accetpable. But
> cardXX is way too generic.
> 
> Perhaps you can explain what "card" is in this context? What is its main
> function?

I provide support for new modular systems which could be equipped with the
different types of replaceable line cards and management board.
The first type of line card is 16x100GbE QSFP28 Ethernet ports.
It equipped with Lattice CPLD device aimed for system and ASIC control, Nvidia FPGA,
Nvidia gearboxes (PHYs).

After that we'll have other line cards: 8x200Gbe QSFP28 Eth ports, 4x400Gbe Eth QSFP-DD,
smart cards equipped with Nvidia ARM CPU for offloading and for fast access to the storage
(EBoF).

For this modular system CPLDs are connected through I2C bus and LED driver will work
over I2C. On main plane CPLD is connected through LPC, and LED driver works over LPC.

Card is common name. But I'd like to avoid some more specific names.

For example, we have huge InfiniBand modular systems with 800 of IB 200G and 400G
ports, which I didn't have yet in upstream. Those system have also replaceable line cards
(so called leaves) and replaceable fabric cards (so called spines).
If I'll give name 'card<bus#>, it will be good also for those systems.
If I'll give more specific name, it'll be not so good name for fabric cards.

I can use name 'fru' instead of 'card' which is standard name and stands for
'Filed Replicable Unit'. Such name will be good for line cards, for fabric cards,
for power units, FANs etcetera.

> 
> Best regards,
> 									Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

Hi!

> > And yes, if you have some kind of device with a status LED, then you can put
> > that into the first card. For example sda::status would be accetpable. But
> > cardXX is way too generic.
> > 
> > Perhaps you can explain what "card" is in this context? What is its main
> > function?
> 
> I provide support for new modular systems which could be equipped with the
> different types of replaceable line cards and management board.
> The first type of line card is 16x100GbE QSFP28 Ethernet ports.
> It equipped with Lattice CPLD device aimed for system and ASIC control, Nvidia FPGA,
> Nvidia gearboxes (PHYs).
> 
> After that we'll have other line cards: 8x200Gbe QSFP28 Eth ports, 4x400Gbe Eth QSFP-DD,
> smart cards equipped with Nvidia ARM CPU for offloading and for fast access to the storage
> (EBoF).
> 
> For this modular system CPLDs are connected through I2C bus and LED driver will work
> over I2C. On main plane CPLD is connected through LPC, and LED driver works over LPC.
> 
> Card is common name. But I'd like to avoid some more specific names.
> 
> For example, we have huge InfiniBand modular systems with 800 of IB 200G and 400G
> ports, which I didn't have yet in upstream. Those system have also replaceable line cards
> (so called leaves) and replaceable fabric cards (so called spines).
> If I'll give name 'card<bus#>, it will be good also for those systems.
> If I'll give more specific name, it'll be not so good name for
 > fabric cards.

Ok, I guess card<something> is best suggestion so far. If it is really
status for specific PCI card, then that's the right way to name it.

Maybe "pciecard<>" would be even better.

Plus we really should have documentation somewhere so that next
person trying to name a LED on a card ends up with same answer.

> I can use name 'fru' instead of 'card' which is standard name and stands for
> 'Filed Replicable Unit'. Such name will be good for line cards, for fabric cards,
> for power units, FANs etcetera.

Best regards,

									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Pavel Machek <pavel@ucw.cz>
> Sent: Wednesday, November 25, 2020 1:20 PM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: Marek Behun <marek.behun@nic.cz>; jacek.anaszewski@gmail.com; linux-
> leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> Hi!
> 
> > > And yes, if you have some kind of device with a status LED, then you
> > > can put that into the first card. For example sda::status would be
> > > accetpable. But cardXX is way too generic.
> > >
> > > Perhaps you can explain what "card" is in this context? What is its
> > > main function?
> >
> > I provide support for new modular systems which could be equipped with
> > the different types of replaceable line cards and management board.
> > The first type of line card is 16x100GbE QSFP28 Ethernet ports.
> > It equipped with Lattice CPLD device aimed for system and ASIC
> > control, Nvidia FPGA, Nvidia gearboxes (PHYs).
> >
> > After that we'll have other line cards: 8x200Gbe QSFP28 Eth ports,
> > 4x400Gbe Eth QSFP-DD, smart cards equipped with Nvidia ARM CPU for
> > offloading and for fast access to the storage (EBoF).
> >
> > For this modular system CPLDs are connected through I2C bus and LED
> > driver will work over I2C. On main plane CPLD is connected through LPC, and
> LED driver works over LPC.
> >
> > Card is common name. But I'd like to avoid some more specific names.
> >
> > For example, we have huge InfiniBand modular systems with 800 of IB
> > 200G and 400G ports, which I didn't have yet in upstream. Those system
> > have also replaceable line cards (so called leaves) and replaceable fabric
> cards (so called spines).
> > If I'll give name 'card<bus#>, it will be good also for those systems.
> > If I'll give more specific name, it'll be not so good name for
>  > fabric cards.
> 
> Ok, I guess card<something> is best suggestion so far. If it is really status for
> specific PCI card, then that's the right way to name it.
> 
> Maybe "pciecard<>" would be even better.

Hi Pavel,

Thank you for reply.

I'd like to avoid 'pci' in name.
Line card can be connected to the different kinds of fabric. It could be for example
InfiniBand or NVlink connectivity.

Also LED itself is accessed through I2C bus.

If you are OK with card<something>, where something in this case is I2C bus number,
I'll send v2 patch with this change.

Thanks,
Vadim.

> 
> Plus we really should have documentation somewhere so that next person
> trying to name a LED on a card ends up with same answer.
> 
> > I can use name 'fru' instead of 'card' which is standard name and
> > stands for 'Filed Replicable Unit'. Such name will be good for line
> > cards, for fabric cards, for power units, FANs etcetera.
> 
> Best regards,
> 
> 									Pavel
> --
> http://www.livejournal.com/~pavelmachek

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2581 bytes --]

Hi!

> > > > And yes, if you have some kind of device with a status LED, then you
> > > > can put that into the first card. For example sda::status would be
> > > > accetpable. But cardXX is way too generic.
> > > >
> > > > Perhaps you can explain what "card" is in this context? What is its
> > > > main function?
> > >
> > > I provide support for new modular systems which could be equipped with
> > > the different types of replaceable line cards and management board.
> > > The first type of line card is 16x100GbE QSFP28 Ethernet ports.
> > > It equipped with Lattice CPLD device aimed for system and ASIC
> > > control, Nvidia FPGA, Nvidia gearboxes (PHYs).
> > >
> > > After that we'll have other line cards: 8x200Gbe QSFP28 Eth ports,
> > > 4x400Gbe Eth QSFP-DD, smart cards equipped with Nvidia ARM CPU for
> > > offloading and for fast access to the storage (EBoF).
> > >
> > > For this modular system CPLDs are connected through I2C bus and LED
> > > driver will work over I2C. On main plane CPLD is connected through LPC, and
> > LED driver works over LPC.
> > >
> > > Card is common name. But I'd like to avoid some more specific names.
> > >
> > > For example, we have huge InfiniBand modular systems with 800 of IB
> > > 200G and 400G ports, which I didn't have yet in upstream. Those system
> > > have also replaceable line cards (so called leaves) and replaceable fabric
> > cards (so called spines).
> > > If I'll give name 'card<bus#>, it will be good also for those systems.
> > > If I'll give more specific name, it'll be not so good name for
> >  > fabric cards.
> > 
> > Ok, I guess card<something> is best suggestion so far. If it is really status for
> > specific PCI card, then that's the right way to name it.
> > 
> > Maybe "pciecard<>" would be even better.
> 
> Hi Pavel,
> 
> Thank you for reply.
> 
> I'd like to avoid 'pci' in name.
> Line card can be connected to the different kinds of fabric. It could be for example
> InfiniBand or NVlink connectivity.
> 
> Also LED itself is accessed through I2C bus.
> 
> If you are OK with card<something>, where something in this case is I2C bus number,
> I'll send v2 patch with this change.

I'd prefer to have something kind of unique. Not every pcie card has
i2c bus on it. If card is on pcie it would be best to name it as it is
common for pcie. InfiniBand would be named according to ib
conventions, etc...

If that is impossible, I guess we can live with card<something> but...

									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Marek Behun]

> > 
> > Vadim, the LED core constructs names in form
> >   device:color:function-enumerator
> > so if you must have number there, IMO it should be
> >   mlxreg:green:status-48
> >   mlxreg:green:status-56
> >   ...  
> 
> But why you consider it as function enumerator?
> For example card48, card56 are two different devices
> of same type.
> Both have 'status' LED.

OK this is a fair point.

I was thinking such because in my mind I had this idea that for an
ethernet switch with interfaces lan0 - lan4 it would make sense to use
the LED_FUNCTION_LAN function with function enumerator. But thinking
about this now again makes me wonder if instead the lan0 - lan4 should
be devicenames instead, since normally they are network interface names.

Vadim, the reason why Pavel and I think that mlxreg (or mlxregN) is not
valid devicename part (although mlxreg has to stay since many users
already depend on it, as you say), is that the mlxreg name is not
exposed anywhere else in Linux from userspace point of view.

Devicename eth0 is okay, because it is network interface name.
Devicename sda would be okay, because everyone knows it is a block
device and you can access it via /dev/sda.
Devicename hci0 would be okay because it is bluetooth interface
accessible via hcitool.
Devicenames mtd0, kbd0, mouse0 would be okay, I think.

But mlxreg is not accessible via anything else in the system. Unless
your systems also have something like /dev/mlxreg, that is.

Do the LEDs on these cards only indicate status of the cards
themselves as a whole? Or are there LEDs on these cards dedicated to
their peripherals? For example if there is an ethernet port with LEDs on
one of these cards, the devicename part for these LEDs should be of the
device of that ethernet port, not mlxreg...

Marek

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Marek Behun <marek.behun@nic.cz>
> Sent: Thursday, October 08, 2020 1:32 PM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: Pavel Machek <pavel@ucw.cz>; jacek.anaszewski@gmail.com; linux-
> leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> > >
> > > Vadim, the LED core constructs names in form
> > >   device:color:function-enumerator
> > > so if you must have number there, IMO it should be
> > >   mlxreg:green:status-48
> > >   mlxreg:green:status-56
> > >   ...
> >
> > But why you consider it as function enumerator?
> > For example card48, card56 are two different devices of same type.
> > Both have 'status' LED.
> 
> OK this is a fair point.
> 
> I was thinking such because in my mind I had this idea that for an ethernet
> switch with interfaces lan0 - lan4 it would make sense to use the
> LED_FUNCTION_LAN function with function enumerator. But thinking about
> this now again makes me wonder if instead the lan0 - lan4 should be
> devicenames instead, since normally they are network interface names.
> 
> Vadim, the reason why Pavel and I think that mlxreg (or mlxregN) is not
> valid devicename part (although mlxreg has to stay since many users
> already depend on it, as you say), is that the mlxreg name is not exposed
> anywhere else in Linux from userspace point of view.
> 
> Devicename eth0 is okay, because it is network interface name.
> Devicename sda would be okay, because everyone knows it is a block device
> and you can access it via /dev/sda.
> Devicename hci0 would be okay because it is bluetooth interface accessible
> via hcitool.
> Devicenames mtd0, kbd0, mouse0 would be okay, I think.
> 
> But mlxreg is not accessible via anything else in the system. Unless your
> systems also have something like /dev/mlxreg, that is.
> 
> Do the LEDs on these cards only indicate status of the cards themselves as a
> whole? Or are there LEDs on these cards dedicated to their peripherals? For
> example if there is an ethernet port with LEDs on one of these cards, the
> devicename part for these LEDs should be of the device of that ethernet
> port, not mlxreg...

Hi Marek,

Each line card must have 'status' LED, indicating status of line card itself.
User can set non-green in case some there are some alarms on different devices,
equipped on this line card. It can be set blink during line card initialization.

Line card could be equipped with UID LED. User can set this LED in order to
find physical location of line card. Sometimes it's hard to see the sticker on
chassis.

Line card also equipped with per port LED, but those LEDs are handled by FW.

So, the device in this case is 'line card'.

In my previous reply I suggest name 'fru' stands for the filed replaceable unit.
This is not something, that is exposed in '/dev', but it describes any replaceable
unit within the system.

> 
> Marek

Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

Hi!

> > > But why you consider it as function enumerator?
> > > For example card48, card56 are two different devices of same type.
> > > Both have 'status' LED.
> > 
> > OK this is a fair point.
> > 
> > I was thinking such because in my mind I had this idea that for an ethernet
> > switch with interfaces lan0 - lan4 it would make sense to use the
> > LED_FUNCTION_LAN function with function enumerator. But thinking about
> > this now again makes me wonder if instead the lan0 - lan4 should be
> > devicenames instead, since normally they are network interface names.
> > 
> > Vadim, the reason why Pavel and I think that mlxreg (or mlxregN) is not
> > valid devicename part (although mlxreg has to stay since many users
> > already depend on it, as you say), is that the mlxreg name is not exposed
> > anywhere else in Linux from userspace point of view.
> > 
> > Devicename eth0 is okay, because it is network interface name.
> > Devicename sda would be okay, because everyone knows it is a block device
> > and you can access it via /dev/sda.
> > Devicename hci0 would be okay because it is bluetooth interface accessible
> > via hcitool.
> > Devicenames mtd0, kbd0, mouse0 would be okay, I think.
> > 
> > But mlxreg is not accessible via anything else in the system. Unless your
> > systems also have something like /dev/mlxreg, that is.
> > 
> > Do the LEDs on these cards only indicate status of the cards themselves as a
> > whole? Or are there LEDs on these cards dedicated to their peripherals? For
> > example if there is an ethernet port with LEDs on one of these cards, the
> > devicename part for these LEDs should be of the device of that ethernet
> > port, not mlxreg...
> 
> Hi Marek,
> 
> Each line card must have 'status' LED, indicating status of line card itself.
> User can set non-green in case some there are some alarms on different devices,
> equipped on this line card. It can be set blink during line card initialization.
> 
> Line card could be equipped with UID LED. User can set this LED in order to
> find physical location of line card. Sometimes it's hard to see the sticker on
> chassis.
> 
> Line card also equipped with per port LED, but those LEDs are handled by FW.
> 
> So, the device in this case is 'line card'.
> 
> In my previous reply I suggest name 'fru' stands for the filed replaceable unit.
> This is not something, that is exposed in '/dev', but it describes any replaceable
> unit within the system.

So.. you'd use the LED to locate right PCI card, or the LED would
indicate that whole card is failing, etc...?

Could we use pci00:1b.0 as the device name? (same as lspci). Probably
replace : with _...

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

RE: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems

[Vadim Pasternak]

> -----Original Message-----
> From: Pavel Machek <pavel@ucw.cz>
> Sent: Wednesday, October 21, 2020 11:34 AM
> To: Vadim Pasternak <vadimp@nvidia.com>
> Cc: Marek Behun <marek.behun@nic.cz>; jacek.anaszewski@gmail.com; linux-
> leds@vger.kernel.org
> Subject: Re: [PATCH led-next 1/1] leds: mlxreg: Allow multi-instantiation of
> same name LED for modular systems
> 
> Hi!
> 
> > > > But why you consider it as function enumerator?
> > > > For example card48, card56 are two different devices of same type.
> > > > Both have 'status' LED.
> > >
> > > OK this is a fair point.
> > >
> > > I was thinking such because in my mind I had this idea that for an
> > > ethernet switch with interfaces lan0 - lan4 it would make sense to
> > > use the LED_FUNCTION_LAN function with function enumerator. But
> > > thinking about this now again makes me wonder if instead the lan0 -
> > > lan4 should be devicenames instead, since normally they are network
> interface names.
> > >
> > > Vadim, the reason why Pavel and I think that mlxreg (or mlxregN) is
> > > not valid devicename part (although mlxreg has to stay since many
> > > users already depend on it, as you say), is that the mlxreg name is
> > > not exposed anywhere else in Linux from userspace point of view.
> > >
> > > Devicename eth0 is okay, because it is network interface name.
> > > Devicename sda would be okay, because everyone knows it is a block
> > > device and you can access it via /dev/sda.
> > > Devicename hci0 would be okay because it is bluetooth interface
> > > accessible via hcitool.
> > > Devicenames mtd0, kbd0, mouse0 would be okay, I think.
> > >
> > > But mlxreg is not accessible via anything else in the system. Unless
> > > your systems also have something like /dev/mlxreg, that is.
> > >
> > > Do the LEDs on these cards only indicate status of the cards
> > > themselves as a whole? Or are there LEDs on these cards dedicated to
> > > their peripherals? For example if there is an ethernet port with
> > > LEDs on one of these cards, the devicename part for these LEDs
> > > should be of the device of that ethernet port, not mlxreg...
> >
> > Hi Marek,
> >
> > Each line card must have 'status' LED, indicating status of line card itself.
> > User can set non-green in case some there are some alarms on different
> > devices, equipped on this line card. It can be set blink during line card
> initialization.
> >
> > Line card could be equipped with UID LED. User can set this LED in
> > order to find physical location of line card. Sometimes it's hard to
> > see the sticker on chassis.
> >
> > Line card also equipped with per port LED, but those LEDs are handled by
> FW.
> >
> > So, the device in this case is 'line card'.
> >
> > In my previous reply I suggest name 'fru' stands for the filed replaceable unit.
> > This is not something, that is exposed in '/dev', but it describes any
> > replaceable unit within the system.
> 
> So.. you'd use the LED to locate right PCI card, or the LED would indicate that
> whole card is failing, etc...?
> 
> Could we use pci00:1b.0 as the device name? (same as lspci). Probably replace
> : with _...
> 

Hi Pavel,

Yes, STATU and UID LED indicates whole line card status/location.

Some line cards could be connected through PCIe, but not all.
From chassis management perspective they are connected by I2C.
And CPLD register map is accessed through I2C.

Following your suggestion it could be i2c-{n} as device name (pdev->id is I2C bus):

 		if (priv->pdev->id > 0)
 			sprintf(led_data->led_cdev_name, "%s%d:%s", "i2c-",
 				priv->pdev->id, data->label);
 		else
 			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
 				data->label);

Or bus name could be passed through the 'identity' filed:
		if (priv->pdev->id > 0)
			sprintf(led_data->led_cdev_name, "%s%d:%s", led_pdata->identity,
				priv->pdev->id, data->label);
		else
			sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
				data->label);

Thanks,
Vadim.

> Best regards,
> 								Pavel
> --
> http://www.livejournal.com/~pavelmachek