| Message ID | 1414037002-25528-7-git-send-email-linux@roeck-us.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>: > Some Marvell switches provide chip temperature data. > Add support for reporting it to the dsa infrastructure. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- [snip] > +/* hwmon support ************************************************************/ > + > +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE)) IS_ENABLED(CONFIG_HWMON)? > + > +static ssize_t temp1_input_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct dsa_switch *ds = dev_get_drvdata(dev); > + int temp, ret; > + > + ret = ds->drv->get_temp(ds, &temp); > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%d\n", temp * 1000); > +} > +static DEVICE_ATTR_RO(temp1_input); You probably want the number of temperature sensors to come from the switch driver, and support arbitrary number of temperature sensors? -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/22/2014 09:37 PM, Florian Fainelli wrote: > 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>: >> Some Marvell switches provide chip temperature data. >> Add support for reporting it to the dsa infrastructure. >> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- > [snip] > >> +/* hwmon support ************************************************************/ >> + >> +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE)) > > IS_ENABLED(CONFIG_HWMON)? > Hi Florian, unfortunately, that won't work; I had it initially and got a nice error message from Fengguang's build test bot. Problem is that hwmon can be built as module and the dsa subsystem can be built into the kernel. In that case, hwmon functionality in the driver must be disabled. The above condition covers that case. The nouveau graphics driver has the same problem and uses the same conditional to solve it. An alternative would be to select HWMON in the NET_DSA Kconfig entry; the Broadcom Tigon3 driver does that. I just don't know if that is a good idea. I am open to suggestions. >> + >> +static ssize_t temp1_input_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct dsa_switch *ds = dev_get_drvdata(dev); >> + int temp, ret; >> + >> + ret = ds->drv->get_temp(ds, &temp); >> + if (ret < 0) >> + return ret; >> + >> + return sprintf(buf, "%d\n", temp * 1000); >> +} >> +static DEVICE_ATTR_RO(temp1_input); > > You probably want the number of temperature sensors to come from the > switch driver, and support arbitrary number of temperature sensors? In that case we would need the number of sensors, pass a sensor index, and create a dynamic number of attributes. The code would get much more complex without real benefit unless there is such a chip - and if there is, we can still change the code at that time. I would prefer to keep it simple for now. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 22, 2014 at 10:06:41PM -0700, Guenter Roeck wrote: > On 10/22/2014 09:37 PM, Florian Fainelli wrote: > >You probably want the number of temperature sensors to come from the > >switch driver, and support arbitrary number of temperature sensors? > > In that case we would need the number of sensors, pass a sensor index, > and create a dynamic number of attributes. The code would get much more > complex without real benefit unless there is such a chip - and if there is, > we can still change the code at that time. I would prefer to keep it simple > for now. Better to do it correctly, right from the start. There *will* be such a chip one day, and the person wanting to add it will appreciate the solid foundation (even if that person ends up being you ;). Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/23/2014 01:24 AM, Richard Cochran wrote: > On Wed, Oct 22, 2014 at 10:06:41PM -0700, Guenter Roeck wrote: >> On 10/22/2014 09:37 PM, Florian Fainelli wrote: >>> You probably want the number of temperature sensors to come from the >>> switch driver, and support arbitrary number of temperature sensors? >> >> In that case we would need the number of sensors, pass a sensor index, >> and create a dynamic number of attributes. The code would get much more >> complex without real benefit unless there is such a chip - and if there is, >> we can still change the code at that time. I would prefer to keep it simple >> for now. > > Better to do it correctly, right from the start. There *will* be such > a chip one day, and the person wanting to add it will appreciate the > solid foundation (even if that person ends up being you ;). > That is really a matter of opinion; others say one should only introduce complex infrastructure into the kernel if and when needed. I don't mind changing the code to anticipate multiple sensors, but as I said it would be more complex, obviously I would only have limited means to test it, and, yes, by nature I tend to be one of the people who prefer to keep things simple. Before I jump into doing this, I would prefer to get guidance from the maintainer. David ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>+static DEVICE_ATTR_RO(temp1_input); > > > >You probably want the number of temperature sensors to come from the > >switch driver, and support arbitrary number of temperature sensors? > > In that case we would need the number of sensors, pass a sensor index, > and create a dynamic number of attributes. The code would get much more > complex without real benefit unless there is such a chip We are two different use cases here: One switch chip with multiple temperature sensors Multiple switch chips, each with its own temperature sensor. I don't know of any hardware using either of these uses cases, but they could appear in the future. If we are not doing the generic implementation now, how about avoiding an ABI break in the future, by hard coding the sensors with .0.0 on the end? Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 23, 2014 at 03:47:06PM +0200, Andrew Lunn wrote: > > >>+static DEVICE_ATTR_RO(temp1_input); > > > > > >You probably want the number of temperature sensors to come from the > > >switch driver, and support arbitrary number of temperature sensors? > > > > In that case we would need the number of sensors, pass a sensor index, > > and create a dynamic number of attributes. The code would get much more > > complex without real benefit unless there is such a chip > > We are two different use cases here: > > One switch chip with multiple temperature sensors I understand this case. However, quite frankly, I consider this quite unlikely to happen. > Multiple switch chips, each with its own temperature sensor. > I don't really see the problem. My understanding is that each of those switch chips will instantiate itself, dsa_switch_setup will be called, which will create a new hwmon device with its own sensors. That is how all other hwmon devices do it, and it works just fine. Why would that approach not work for switches in the dsa infrastructure ? Am I missing something ? If the concern or assumption is that there can not be more than one "temp1_input" attribute, here is an example from a system with a large number of chips with temperature sensors. root@evo-xb49:/sys/class/hwmon# ls hwmon*/temp1_input hwmon1/temp1_input hwmon22/temp1_input hwmon35/temp1_input hwmon10/temp1_input hwmon23/temp1_input hwmon36/temp1_input hwmon11/temp1_input hwmon24/temp1_input hwmon37/temp1_input hwmon12/temp1_input hwmon25/temp1_input hwmon38/temp1_input hwmon13/temp1_input hwmon26/temp1_input hwmon39/temp1_input hwmon14/temp1_input hwmon27/temp1_input hwmon4/temp1_input hwmon15/temp1_input hwmon28/temp1_input hwmon40/temp1_input hwmon16/temp1_input hwmon29/temp1_input hwmon5/temp1_input hwmon17/temp1_input hwmon3/temp1_input hwmon6/temp1_input hwmon18/temp1_input hwmon30/temp1_input hwmon7/temp1_input hwmon19/temp1_input hwmon31/temp1_input hwmon8/temp1_input hwmon2/temp1_input hwmon32/temp1_input hwmon9/temp1_input hwmon20/temp1_input hwmon33/temp1_input hwmon21/temp1_input hwmon34/temp1_input There are 6xDS1625, 11xDS1721, 1xLM95234, 4xMAX6697, and 17xLTC2978 in this system if I counted correctly. That doesn't mean that the drivers need to do anything special for their multiple instances. Also, the "name" of each instance does not have to be unique. The "name" reflects the driver name, not its instance. root@evo-xb49:/sys/class/hwmon# grep . */name | grep max hwmon20/name:max6697 hwmon21/name:max6697 hwmon22/name:max6697 hwmon23/name:max6697 > I don't know of any hardware using either of these uses cases, but > they could appear in the future. > > If we are not doing the generic implementation now, how about avoiding > an ABI break in the future, by hard coding the sensors with .0.0 on > the end? I am a little lost. What would that be for, and what would the ABI breakage be ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 23, 2014 at 09:27:55AM -0700, Guenter Roeck wrote: > On Thu, Oct 23, 2014 at 03:47:06PM +0200, Andrew Lunn wrote: > > > >>+static DEVICE_ATTR_RO(temp1_input); > > > > > > > >You probably want the number of temperature sensors to come from the > > > >switch driver, and support arbitrary number of temperature sensors? > > > > > > In that case we would need the number of sensors, pass a sensor index, > > > and create a dynamic number of attributes. The code would get much more > > > complex without real benefit unless there is such a chip > > > > We are two different use cases here: > > > > One switch chip with multiple temperature sensors > > I understand this case. However, quite frankly, I consider this quite > unlikely to happen. > > > Multiple switch chips, each with its own temperature sensor. > > > I don't really see the problem. My understanding is that each of those > switch chips will instantiate itself, dsa_switch_setup will be called, > which will create a new hwmon device with its own sensors. That is > how all other hwmon devices do it, and it works just fine. > Why would that approach not work for switches in the dsa infrastructure ? > Am I missing something ? > > If the concern or assumption is that there can not be more than one > "temp1_input" attribute, here is an example from a system with a large > number of chips with temperature sensors. > > root@evo-xb49:/sys/class/hwmon# ls hwmon*/temp1_input > hwmon1/temp1_input hwmon22/temp1_input hwmon35/temp1_input > hwmon10/temp1_input hwmon23/temp1_input hwmon36/temp1_input > hwmon11/temp1_input hwmon24/temp1_input hwmon37/temp1_input > hwmon12/temp1_input hwmon25/temp1_input hwmon38/temp1_input > hwmon13/temp1_input hwmon26/temp1_input hwmon39/temp1_input > hwmon14/temp1_input hwmon27/temp1_input hwmon4/temp1_input > hwmon15/temp1_input hwmon28/temp1_input hwmon40/temp1_input > hwmon16/temp1_input hwmon29/temp1_input hwmon5/temp1_input > hwmon17/temp1_input hwmon3/temp1_input hwmon6/temp1_input > hwmon18/temp1_input hwmon30/temp1_input hwmon7/temp1_input > hwmon19/temp1_input hwmon31/temp1_input hwmon8/temp1_input > hwmon2/temp1_input hwmon32/temp1_input hwmon9/temp1_input > hwmon20/temp1_input hwmon33/temp1_input > hwmon21/temp1_input hwmon34/temp1_input So are you saying it is impossible to map a hwmon device to a physical sensor? I can know there is a hotspot somewhere in my machine, but it is impossible to figure where that hot spot is using hwmon? > > If we are not doing the generic implementation now, how about avoiding > > an ABI break in the future, by hard coding the sensors with .0.0 on > > the end? > > I am a little lost. What would that be for, and what would the ABI breakage > be ? I assumed you would want to be able to map a temperature sensor to a switch package. You want a unique identifier, maybe its hwman name? So name "dsa.0.0" would be the temperature sensor 0 on switch 0. If we don't name them like this now, whenever somebody does add support for this will cause that name to change and we have an ABI break. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 23, 2014 at 06:54:59PM +0200, Andrew Lunn wrote: > On Thu, Oct 23, 2014 at 09:27:55AM -0700, Guenter Roeck wrote: > > On Thu, Oct 23, 2014 at 03:47:06PM +0200, Andrew Lunn wrote: > > > > >>+static DEVICE_ATTR_RO(temp1_input); > > > > > > > > > >You probably want the number of temperature sensors to come from the > > > > >switch driver, and support arbitrary number of temperature sensors? > > > > > > > > In that case we would need the number of sensors, pass a sensor index, > > > > and create a dynamic number of attributes. The code would get much more > > > > complex without real benefit unless there is such a chip > > > > > > We are two different use cases here: > > > > > > One switch chip with multiple temperature sensors > > > > I understand this case. However, quite frankly, I consider this quite > > unlikely to happen. > > > > > Multiple switch chips, each with its own temperature sensor. > > > > > I don't really see the problem. My understanding is that each of those > > switch chips will instantiate itself, dsa_switch_setup will be called, > > which will create a new hwmon device with its own sensors. That is > > how all other hwmon devices do it, and it works just fine. > > Why would that approach not work for switches in the dsa infrastructure ? > > Am I missing something ? > > > > If the concern or assumption is that there can not be more than one > > "temp1_input" attribute, here is an example from a system with a large > > number of chips with temperature sensors. > > > > root@evo-xb49:/sys/class/hwmon# ls hwmon*/temp1_input > > hwmon1/temp1_input hwmon22/temp1_input hwmon35/temp1_input > > hwmon10/temp1_input hwmon23/temp1_input hwmon36/temp1_input > > hwmon11/temp1_input hwmon24/temp1_input hwmon37/temp1_input > > hwmon12/temp1_input hwmon25/temp1_input hwmon38/temp1_input > > hwmon13/temp1_input hwmon26/temp1_input hwmon39/temp1_input > > hwmon14/temp1_input hwmon27/temp1_input hwmon4/temp1_input > > hwmon15/temp1_input hwmon28/temp1_input hwmon40/temp1_input > > hwmon16/temp1_input hwmon29/temp1_input hwmon5/temp1_input > > hwmon17/temp1_input hwmon3/temp1_input hwmon6/temp1_input > > hwmon18/temp1_input hwmon30/temp1_input hwmon7/temp1_input > > hwmon19/temp1_input hwmon31/temp1_input hwmon8/temp1_input > > hwmon2/temp1_input hwmon32/temp1_input hwmon9/temp1_input > > hwmon20/temp1_input hwmon33/temp1_input > > hwmon21/temp1_input hwmon34/temp1_input > > So are you saying it is impossible to map a hwmon device to a physical > sensor? I can know there is a hotspot somewhere in my machine, but it > is impossible to figure where that hot spot is using hwmon? > No, I am not saying that. The hwmon device's parent device will tell, which is how it works for all other hwmon devices. > > > If we are not doing the generic implementation now, how about avoiding > > > an ABI break in the future, by hard coding the sensors with .0.0 on > > > the end? > > > > I am a little lost. What would that be for, and what would the ABI breakage > > be ? > > I assumed you would want to be able to map a temperature sensor to a > switch package. You want a unique identifier, maybe its hwman name? So > name "dsa.0.0" would be the temperature sensor 0 on switch 0. If we > don't name them like this now, whenever somebody does add support for > this will cause that name to change and we have an ABI break. > Not really. Again, the parent device provides that information. libsensors, which is the preferred way of accessing sensors information from user space, provides the parent device instance as part of the logical sensor device name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001, and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001, and so on. I don't see how this would add any value. Also, temperature sensor 2 on switch 1 would be named temp2_input. There would not be a sepate hwmon device for the same chip. This is all quite similar to, say, the CPU temperature, which is reported by the sensors command as coretemp-isa-0000 Adapter: ISA adapter Physical id 0: +28.0°C (high = +80.0°C, crit = +100.0°C) Core 0: +28.0°C (high = +80.0°C, crit = +100.0°C) Core 1: +24.0°C (high = +80.0°C, crit = +100.0°C) Core 2: +24.0°C (high = +80.0°C, crit = +100.0°C) Core 3: +26.0°C (high = +80.0°C, crit = +100.0°C) on my system at home. The raw data in this case is name:coretemp temp1_crit:100000 temp1_crit_alarm:0 temp1_input:31000 temp1_label:Physical id 0 temp1_max:80000 temp2_crit:100000 temp2_crit_alarm:0 temp2_input:31000 temp2_label:Core 0 temp2_max:80000 temp3_crit:100000 temp3_crit_alarm:0 temp3_input:26000 temp3_label:Core 1 temp3_max:80000 temp4_crit:100000 temp4_crit_alarm:0 temp4_input:24000 temp4_label:Core 2 temp4_max:80000 temp5_crit:100000 temp5_crit_alarm:0 temp5_input:27000 temp5_label:Core 3 temp5_max:80000 In this case, the parent device symlink is device -> ../../../coretemp.0 A second CPU in a multi-CPU server system would have pretty much the same information in its hwmon device directory, except that the parent device would point to coretemp.1, and the sensors command would display "coretemp-isa-0001". The "name" attribute for that second CPU's hwmon device node would still be "coretemp". Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> No, I am not saying that. The hwmon device's parent device will tell, > which is how it works for all other hwmon devices. O.K, so parent is important. > Not really. Again, the parent device provides that information. libsensors, > which is the preferred way of accessing sensors information from user space, > provides the parent device instance as part of the logical sensor device > name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001, > and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001, > and so on. I don't see how this would add any value. isa is the name of the ethernet device? Why is it not eth0? Most Marvell SoCs used in WiFi Access Points have multiple ethernet interfaces, so i would hope the parent actually identifies which ethernet interface it is hanging off. Now consider the example in http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/net/dsa/dsa.txt We have two switches hanging off one ethernet interface. What will the naming look like in this case? The Marvell DSA tagging scheme allows you to have 16 switches hanging off one ethernet interface. How is the naming going to work then, especially if there is a mixture of switch chips, some with temperature sensors, and some without? What would really help is if each switch has a device in the linux device model. The hwmon parent would then be the switch device. The EEPROM would then hang off the switch device, not an interface on the switch device, etc. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 23, 2014 at 08:03:57PM +0200, Andrew Lunn wrote: > > No, I am not saying that. The hwmon device's parent device will tell, > > which is how it works for all other hwmon devices. > > O.K, so parent is important. > > > Not really. Again, the parent device provides that information. libsensors, > > which is the preferred way of accessing sensors information from user space, > > provides the parent device instance as part of the logical sensor device > > name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001, > > and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001, > > and so on. I don't see how this would add any value. > > isa is the name of the ethernet device? Why is it not eth0? Most isa is just an internal name made up by libsensors, and pretty much just indicates something like "neither i2c nor spi". It is mostly historic, but nowadays almost part of the ABI. It is made up by user space, based on the parent device type, not by the kernel. > Marvell SoCs used in WiFi Access Points have multiple ethernet > interfaces, so i would hope the parent actually identifies which > ethernet interface it is hanging off. > > Now consider the example in > > http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/net/dsa/dsa.txt > > We have two switches hanging off one ethernet interface. What will the > naming look like in this case? > Ah, that is a good question. You are right, the parent will be the same for both of those switches but should be unique. Hmm ... > The Marvell DSA tagging scheme allows you to have 16 switches hanging > off one ethernet interface. How is the naming going to work then, > especially if there is a mixture of switch chips, some with > temperature sensors, and some without? > The ones without sensor would not create a hwmon device, so that should not be an issue. You are right, the other use cases are more tricky. > What would really help is if each switch has a device in the linux > device model. The hwmon parent would then be the switch device. The > EEPROM would then hang off the switch device, not an interface on the > switch device, etc. Good point. Unfortunately that is not the case. You have convinced me that 'dsa' as hwmon attribute name is insufficient, so let's see what we have. - the parent network device in dst->master_netdev - the dsa device in 'parent' - the host device in host_dev - the switch index in 'index' First question is what should be the parent device. We have three to choose from. Should it be the parent network device or the dsa device ? The dsa device seems like a better choice to me, but I am open to suggestions. A problem with choosing the network device as parent may be that this device could by itself have a temperature sensor (some Intel and broadcom Ethernet chips have that), which could cause confusion. Second is what to choose for the hwmon device 'name' attribute. 'dsa' is not sufficient, but what to choose instead ? dsa.X or dsa_X, where X is the switch index ? At this point I am open to suggestions. Note that we can not use the name returned from the switch probe functions as it may contain spaces and other invalid characters. Including the ethernet device name (eg as in eth0_dsa_0) may also be problematic if it can contain '-', which is illegal for hwmon devices. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 23, 2014 at 11:43:22AM -0700, Guenter Roeck wrote: > On Thu, Oct 23, 2014 at 08:03:57PM +0200, Andrew Lunn wrote: > > > No, I am not saying that. The hwmon device's parent device will tell, > > > which is how it works for all other hwmon devices. > > > > O.K, so parent is important. > > > > > Not really. Again, the parent device provides that information. libsensors, > > > which is the preferred way of accessing sensors information from user space, > > > provides the parent device instance as part of the logical sensor device > > > name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001, > > > and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001, > > > and so on. I don't see how this would add any value. > > > > isa is the name of the ethernet device? Why is it not eth0? Most > > isa is just an internal name made up by libsensors, and pretty much just > indicates something like "neither i2c nor spi". It is mostly historic, > but nowadays almost part of the ABI. It is made up by user space, > based on the parent device type, not by the kernel. So for DSA, since it is not i2c or spi, the parent is actually irrelevant, because libsensor ignores it and says "IsSomethingAlien". So the name alone needs to identify it. > You have convinced me that 'dsa' as hwmon attribute name is insufficient, > so let's see what we have. > > - the parent network device in dst->master_netdev > - the dsa device in 'parent' > - the host device in host_dev > - the switch index in 'index' > > First question is what should be the parent device. Does it even matter, given the observation above? I would probably go for dsa, since as you said, the Ethernet device may have a temperature sensor of its own. DSA is a virtual device... > Second is what to choose for the hwmon device 'name' attribute. > 'dsa' is not sufficient, but what to choose instead ? dsa.X or dsa_X, > where X is the switch index ? At this point I am open to suggestions. > Note that we can not use the name returned from the switch probe > functions as it may contain spaces and other invalid characters. > Including the ethernet device name (eg as in eth0_dsa_0) may also be > problematic if it can contain '-', which is illegal for hwmon devices. Does hwmon offer a function to sanitise a string? The switch index definitely should be used and i would probably combine that with a sanitised version of the ethernet device name and "dsa". Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Guenter Roeck <linux@roeck-us.net> Date: Wed, 22 Oct 2014 22:06:41 -0700 > On 10/22/2014 09:37 PM, Florian Fainelli wrote: >> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>: >>> Some Marvell switches provide chip temperature data. >>> Add support for reporting it to the dsa infrastructure. >>> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> --- >> [snip] >> >>> +/* hwmon support >>> ************************************************************/ >>> + >>> +#if defined(CONFIG_HWMON) || (defined(MODULE) && >>> defined(CONFIG_HWMON_MODULE)) >> >> IS_ENABLED(CONFIG_HWMON)? >> > > Hi Florian, > > unfortunately, that won't work; I had it initially and got a nice > error message > from Fengguang's build test bot. Then the Kconfig dependencies are broken. Fix Kconfig to only allow legal combinations. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/23/2014 10:03 PM, David Miller wrote: > From: Guenter Roeck <linux@roeck-us.net> > Date: Wed, 22 Oct 2014 22:06:41 -0700 > >> On 10/22/2014 09:37 PM, Florian Fainelli wrote: >>> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>: >>>> Some Marvell switches provide chip temperature data. >>>> Add support for reporting it to the dsa infrastructure. >>>> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>> --- >>> [snip] >>> >>>> +/* hwmon support >>>> ************************************************************/ >>>> + >>>> +#if defined(CONFIG_HWMON) || (defined(MODULE) && >>>> defined(CONFIG_HWMON_MODULE)) >>> >>> IS_ENABLED(CONFIG_HWMON)? >>> >> >> Hi Florian, >> >> unfortunately, that won't work; I had it initially and got a nice >> error message >> from Fengguang's build test bot. > > Then the Kconfig dependencies are broken. > > Fix Kconfig to only allow legal combinations. > I see two options for that: - Add select HWMON to the NET_DSA Kconfig entry. Example is Broadcom TIGON3 driver. - Add a DSA_HWMON Kconfig entry to define the dependencies and to let the user select if the functionality should be enabled. Example is Intel IGB driver. Any preference from your side ? If no, I'll go with the latter. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/24/2014 05:52 AM, Florian Fainelli wrote: > Le 23/10/2014 22:40, Guenter Roeck a écrit : >> On 10/23/2014 10:03 PM, David Miller wrote: >>> From: Guenter Roeck <linux@roeck-us.net> >>> Date: Wed, 22 Oct 2014 22:06:41 -0700 >>> >>>> On 10/22/2014 09:37 PM, Florian Fainelli wrote: >>>>> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>: >>>>>> Some Marvell switches provide chip temperature data. >>>>>> Add support for reporting it to the dsa infrastructure. >>>>>> >>>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>>>> --- >>>>> [snip] >>>>> >>>>>> +/* hwmon support >>>>>> ************************************************************/ >>>>>> + >>>>>> +#if defined(CONFIG_HWMON) || (defined(MODULE) && >>>>>> defined(CONFIG_HWMON_MODULE)) >>>>> >>>>> IS_ENABLED(CONFIG_HWMON)? >>>>> >>>> >>>> Hi Florian, >>>> >>>> unfortunately, that won't work; I had it initially and got a nice >>>> error message >>>> from Fengguang's build test bot. >>> >>> Then the Kconfig dependencies are broken. >>> >>> Fix Kconfig to only allow legal combinations. >>> >> >> I see two options for that: >> >> - Add >> select HWMON >> to the NET_DSA Kconfig entry. >> Example is Broadcom TIGON3 driver. >> >> - Add a DSA_HWMON Kconfig entry to define the dependencies and >> to let the user select if the functionality should be enabled. >> Example is Intel IGB driver. >> >> Any preference from your side ? If no, I'll go with the latter. > > I would prefer DSA_HWMON personaly, though no strong feelings. That is what I ended up implementing. NET_DSA_HWMON, actually, for consistency. Since this is the most debated patch in this patch set, how about you drop it from your v2 and we sort this one out separately? We can do that if there are still issues in v2. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Guenter Roeck <linux@roeck-us.net> Date: Thu, 23 Oct 2014 22:40:59 -0700 > I see two options for that: > > - Add > select HWMON > to the NET_DSA Kconfig entry. > Example is Broadcom TIGON3 driver. > > - Add a DSA_HWMON Kconfig entry to define the dependencies and > to let the user select if the functionality should be enabled. > Example is Intel IGB driver. > > Any preference from your side ? If no, I'll go with the latter. Probably the latter is better, select can get you into trouble. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 23/10/2014 22:40, Guenter Roeck a écrit : > On 10/23/2014 10:03 PM, David Miller wrote: >> From: Guenter Roeck <linux@roeck-us.net> >> Date: Wed, 22 Oct 2014 22:06:41 -0700 >> >>> On 10/22/2014 09:37 PM, Florian Fainelli wrote: >>>> 2014-10-22 21:03 GMT-07:00 Guenter Roeck <linux@roeck-us.net>: >>>>> Some Marvell switches provide chip temperature data. >>>>> Add support for reporting it to the dsa infrastructure. >>>>> >>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>>>> --- >>>> [snip] >>>> >>>>> +/* hwmon support >>>>> ************************************************************/ >>>>> + >>>>> +#if defined(CONFIG_HWMON) || (defined(MODULE) && >>>>> defined(CONFIG_HWMON_MODULE)) >>>> >>>> IS_ENABLED(CONFIG_HWMON)? >>>> >>> >>> Hi Florian, >>> >>> unfortunately, that won't work; I had it initially and got a nice >>> error message >>> from Fengguang's build test bot. >> >> Then the Kconfig dependencies are broken. >> >> Fix Kconfig to only allow legal combinations. >> > > I see two options for that: > > - Add > select HWMON > to the NET_DSA Kconfig entry. > Example is Broadcom TIGON3 driver. > > - Add a DSA_HWMON Kconfig entry to define the dependencies and > to let the user select if the functionality should be enabled. > Example is Intel IGB driver. > > Any preference from your side ? If no, I'll go with the latter. I would prefer DSA_HWMON personaly, though no strong feelings. Since this is the most debated patch in this patch set, how about you drop it from your v2 and we sort this one out separately? -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/23/2014 12:55 PM, Andrew Lunn wrote: [ ... ] > > Does hwmon offer a function to sanitise a string? > No, that wasn't necessary so far. The 'name' string is a constant string provided by the driver. > The switch index definitely should be used and i would probably > combine that with a sanitised version of the ethernet device name and > "dsa". > How is this ? em1dsa0-isa-0000 Adapter: ISA adapter temp1: +38.0°C (high = +100.0°C) This is the sanitized network device name + 'dsa' + index. Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 23, 2014 at 09:55:26PM +0200, Andrew Lunn wrote: > On Thu, Oct 23, 2014 at 11:43:22AM -0700, Guenter Roeck wrote: > > On Thu, Oct 23, 2014 at 08:03:57PM +0200, Andrew Lunn wrote: > > > > No, I am not saying that. The hwmon device's parent device will tell, > > > > which is how it works for all other hwmon devices. > > > > > > O.K, so parent is important. > > > > > > > Not really. Again, the parent device provides that information. libsensors, > > > > which is the preferred way of accessing sensors information from user space, > > > > provides the parent device instance as part of the logical sensor device > > > > name. In this case, the names will end up being dsa-isa-0000, dsa-isa-0001, > > > > and so on. With your added tags it would be dsa.0.0-isa-0000, dsa.0.1-isa-0001, > > > > and so on. I don't see how this would add any value. > > > > > > isa is the name of the ethernet device? Why is it not eth0? Most > > > > isa is just an internal name made up by libsensors, and pretty much just > > indicates something like "neither i2c nor spi". It is mostly historic, > > but nowadays almost part of the ABI. It is made up by user space, > > based on the parent device type, not by the kernel. > > So for DSA, since it is not i2c or spi, the parent is actually > irrelevant, because libsensor ignores it and says "IsSomethingAlien". > So the name alone needs to identify it. > > > You have convinced me that 'dsa' as hwmon attribute name is insufficient, > > so let's see what we have. > > > > - the parent network device in dst->master_netdev > > - the dsa device in 'parent' > > - the host device in host_dev > > - the switch index in 'index' > > > > First question is what should be the parent device. > > Does it even matter, given the observation above? I would probably go > for dsa, since as you said, the Ethernet device may have a temperature > sensor of its own. DSA is a virtual device... > > > Second is what to choose for the hwmon device 'name' attribute. > > 'dsa' is not sufficient, but what to choose instead ? dsa.X or dsa_X, > > where X is the switch index ? At this point I am open to suggestions. > > Note that we can not use the name returned from the switch probe > > functions as it may contain spaces and other invalid characters. > > Including the ethernet device name (eg as in eth0_dsa_0) may also be > > problematic if it can contain '-', which is illegal for hwmon devices. > > Does hwmon offer a function to sanitise a string? > > The switch index definitely should be used and i would probably > combine that with a sanitised version of the ethernet device name and > "dsa". > Here is another naming option: em1dsa0-virtual-0 Adapter: Virtual device temp1: +52.0°C (high = +100.0°C) I think I'll go with that one. I get it by not specifying a parent device when registering with the hwmon subsystem. It is kind of similar to the thermal sensor data reported through acpi. acpitz-virtual-0 Adapter: Virtual device temp1: +52.0°C (crit = +111.0°C) temp2: +52.0°C (crit = +111.0°C) Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Here is another naming option: > > em1dsa0-virtual-0 I prefer this over isa. However, i think there should be some sort of separator between the network device name and dsa. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/25/14 07:01, Andrew Lunn wrote: >> Here is another naming option: >> >> em1dsa0-virtual-0 > > I prefer this over isa. > > However, i think there should be some sort of separator between the > network device name and dsa. Considering that network devices can be renamed, do we want it to be included in the sensor name at all? -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/25/14 07:01, Andrew Lunn wrote: >> Here is another naming option: >> >> em1dsa0-virtual-0 > > I prefer this over isa. > > However, i think there should be some sort of separator between the > network device name and dsa. Considering that network devices can be renamed, do we want it to be included in the sensor name at all? -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 25, 2014 at 10:23:27AM -0700, Florian Fainelli wrote: > On 10/25/14 07:01, Andrew Lunn wrote: > >> Here is another naming option: > >> > >> em1dsa0-virtual-0 > > > > I prefer this over isa. > > > > However, i think there should be some sort of separator between the > > network device name and dsa. > > Considering that network devices can be renamed, do we want it to be > included in the sensor name at all? Well, we need something which identifies the "DSA collection". In that, you could have two or more collections, connected to different network devices. I once came across a PCI board with two Intel ethernet chipsets and two 10 port Marvell switches. Each switch had one host cpu port, 3 ports interconnected to the other switch, and 6 going to the back panel. You would want to describe that as two separate DSA entities, not one DSA with two switches. So the name needs something to indicate the DSA collection. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/25/2014 07:01 AM, Andrew Lunn wrote: >> Here is another naming option: >> >> em1dsa0-virtual-0 > > I prefer this over isa. > > However, i think there should be some sort of separator between the > network device name and dsa. > Fine with me. Any preference ? Note that '-' is not permitted. Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/25/2014 11:00 AM, Andrew Lunn wrote: > On Sat, Oct 25, 2014 at 10:23:27AM -0700, Florian Fainelli wrote: >> On 10/25/14 07:01, Andrew Lunn wrote: >>>> Here is another naming option: >>>> >>>> em1dsa0-virtual-0 >>> >>> I prefer this over isa. >>> >>> However, i think there should be some sort of separator between the >>> network device name and dsa. >> >> Considering that network devices can be renamed, do we want it to be >> included in the sensor name at all? > > Well, we need something which identifies the "DSA collection". In > that, you could have two or more collections, connected to different > network devices. > > I once came across a PCI board with two Intel ethernet chipsets and > two 10 port Marvell switches. Each switch had one host cpu port, 3 > ports interconnected to the other switch, and 6 going to the back > panel. You would want to describe that as two separate DSA entities, > not one DSA with two switches. So the name needs something to indicate > the DSA collection. > I agree. I could use the dsa instance index, so we would have names such as dsa0-isa-0000, dsa1-isa-0000, dsa0-isa-0001, dsa1-isa-0001, and so on, but that looks awkward (I can't do the index with virtual names because they don't have a parent and thus no index). Using the ethernet interface name seems like a good idea to me. Sure it can change, but the name is typically chosen before the dsa device is instantiated, and if it is changed manually later we just can not help it. So, if people want a delimiter between network device name and dsa, what should it be ? Unless I hear otherwise, I'll choose '_'. Guenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/dsa.h b/include/net/dsa.h index b765592..d8b3057 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -242,6 +242,12 @@ struct dsa_switch_driver { struct ethtool_eee *e); int (*get_eee)(struct dsa_switch *ds, int port, struct ethtool_eee *e); + + /* Hardware monitoring */ + int (*get_temp)(struct dsa_switch *ds, int *temp); + int (*get_temp_limit)(struct dsa_switch *ds, int *temp); + int (*set_temp_limit)(struct dsa_switch *ds, int temp); + int (*get_temp_alarm)(struct dsa_switch *ds, bool *alarm); }; void register_switch_driver(struct dsa_switch_driver *type); diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 22f34cf..6567c8e 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -9,6 +9,8 @@ * (at your option) any later version. */ +#include <linux/device.h> +#include <linux/hwmon.h> #include <linux/list.h> #include <linux/platform_device.h> #include <linux/slab.h> @@ -17,6 +19,7 @@ #include <linux/of.h> #include <linux/of_mdio.h> #include <linux/of_platform.h> +#include <linux/sysfs.h> #include "dsa_priv.h" char dsa_driver_version[] = "0.1"; @@ -71,6 +74,104 @@ dsa_switch_probe(struct device *host_dev, int sw_addr, char **_name) return ret; } +/* hwmon support ************************************************************/ + +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE)) + +static ssize_t temp1_input_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct dsa_switch *ds = dev_get_drvdata(dev); + int temp, ret; + + ret = ds->drv->get_temp(ds, &temp); + if (ret < 0) + return ret; + + return sprintf(buf, "%d\n", temp * 1000); +} +static DEVICE_ATTR_RO(temp1_input); + +static ssize_t temp1_max_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct dsa_switch *ds = dev_get_drvdata(dev); + int temp, ret; + + ret = ds->drv->get_temp_limit(ds, &temp); + if (ret < 0) + return ret; + + return sprintf(buf, "%d\n", temp * 1000); +} + +static ssize_t temp1_max_store(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t count) +{ + struct dsa_switch *ds = dev_get_drvdata(dev); + int temp, ret; + + ret = kstrtoint(buf, 0, &temp); + if (ret < 0) + return ret; + + ret = ds->drv->set_temp_limit(ds, DIV_ROUND_CLOSEST(temp, 1000)); + if (ret < 0) + return ret; + + return count; +} +static DEVICE_ATTR(temp1_max, S_IRUGO, temp1_max_show, temp1_max_store); + +static ssize_t temp1_max_alarm_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct dsa_switch *ds = dev_get_drvdata(dev); + bool alarm; + int ret; + + ret = ds->drv->get_temp_alarm(ds, &alarm); + if (ret < 0) + return ret; + + return sprintf(buf, "%d\n", alarm); +} +static DEVICE_ATTR_RO(temp1_max_alarm); + +static struct attribute *dsa_hwmon_attrs[] = { + &dev_attr_temp1_input.attr, /* 0 */ + &dev_attr_temp1_max.attr, /* 1 */ + &dev_attr_temp1_max_alarm.attr, /* 2 */ + NULL +}; + +static umode_t dsa_hwmon_attrs_visible(struct kobject *kobj, + struct attribute *attr, int index) +{ + struct device *dev = container_of(kobj, struct device, kobj); + struct dsa_switch *ds = dev_get_drvdata(dev); + struct dsa_switch_driver *drv = ds->drv; + umode_t mode = attr->mode; + + if (index == 1) { + if (!drv->get_temp_limit) + mode = 0; + else if (drv->set_temp_limit) + mode |= S_IWUSR; + } else if (index == 2 && !drv->get_temp_alarm) { + mode = 0; + } + return mode; +} + +static const struct attribute_group dsa_hwmon_group = { + .attrs = dsa_hwmon_attrs, + .is_visible = dsa_hwmon_attrs_visible, +}; +__ATTRIBUTE_GROUPS(dsa_hwmon); + +#endif /* CONFIG_HWMON */ /* basic switch operations **************************************************/ static struct dsa_switch * @@ -225,6 +326,16 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index, ds->ports[i] = slave_dev; } +#if defined(CONFIG_HWMON) || (defined(MODULE) && defined(CONFIG_HWMON_MODULE)) + /* If the switch provides a temperature sensor, + * register with hardware monitoring subsystem. + * Treat registration error as non-fatal and ignore it. + */ + if (drv->get_temp) + devm_hwmon_device_register_with_groups(parent, "dsa", ds, + dsa_hwmon_groups); +#endif + return ds; out_free:
Some Marvell switches provide chip temperature data. Add support for reporting it to the dsa infrastructure. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- include/net/dsa.h | 6 +++ net/dsa/dsa.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+)