| Message ID | 20201006165850.17790-1-vadimp@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [led-next,1/1] leds: mlxreg: Allow multi-instantiation of same name LED for modular systems | expand |
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;
> -----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;
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
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
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
> -----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
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
> -----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
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
> -----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
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
> -----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
> > > > 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
> -----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
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
> -----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
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
> -----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
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
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;
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(-)