Date:   Thu, 31 Mar 2022 10:11:22 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Michael Walle <michael@...le.cc>
Cc:     Xu Yilun <yilun.xu@...el.com>,
        David Laight <David.Laight@...lab.com>,
        Tom Rix <trix@...hat.com>, Jean Delvare <jdelvare@...e.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

On 3/31/22 07:58, Russell King (Oracle) wrote:
> On Thu, Mar 31, 2022 at 04:51:47PM +0200, Michael Walle wrote:
>> Am 2022-03-31 16:45, schrieb Russell King (Oracle):
>>> On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote:
>>>> Michael, let's just drop the changes outside drivers/hwmon from
>>>> the series, and let's keep hwmon_is_bad_char() in the include file.
>>>> Let's just document it, explaining its use case.
>>>
>>> Why? There hasn't been any objection to the change. All the discussion
>>> seems to be around the new function (this patch) rather than the actual
>>> conversions in drivers.
>>>
>>> I'm entirely in favour of cleaning this up - it irks me that we're doing
>>> exactly the same cleanup everywhere we have a hwmon.
>>>
>>> At the very least, I would be completely in favour of keeping the
>>> changes in the sfp and phy code.
>>
>> FWIW, my plan was to send the hwmon patches first, by then my other
>> series (the polynomial_calc() one) will also be ready to be picked.
>> Then I'd ask Guenter for a stable branch with these two series which
>> hopefully get merged into net-next. Then I can repost the missing
>> patches on net-next along with the new sensors support for the GPY
>> and LAN8814 PHYs.
> 
> Okay, that's fine. It just sounded like the conversion of other drivers
> outside drivers/hwmon was being dropped.
> 

Not dropped, just disconnected. From hwmon perspective, we want a certain
set of characters to be dropped or replaced. Also, from hwmon perspective,
it makes sense to have the helper function allocate the replacement data.
There was disagreement about which characters should be replaced, and if
the helper function should allocate the replacement string or not.
I have no intention to change the set of characters without good reason,
and I feel quite strongly about allocating the replacement in the helper
function. Since potential callers don't _have_ to use the helper and don't
_have_ to provide valid names (and are responsible for the consequences),
I would like that discussion to be separate from hwmon changes.

> Note that there's another "sanitisation" of hwmon names in
> drivers/net/phy/marvell.c - that converts any non-alnum character to
> an underscore. Not sure why the different approach was chosen there.
> 

It actually drops non-alphanumeric characters. The name is derived
from the phy device name, which I think is derived from the name field
in struct phy_driver. That includes spaces and '(', ')'. I honestly
have no idea what libsensors would do with '(' and ')'. Either case,
even if that would create a hiccup in libsensors and we would add
'(' and ')' to the 'forbidden' list of characters, the fact that the
code doesn't replace but drop non-alphanumeric characters means
it won't be able to use a helper anyway since that would result
in a hwmon 'name' attribute change and thus not be backward
compatible. Besides, "Marvell_88E1111__Finisar_" would look a bit
odd anyway, and "Marvell88E1111Finisar" may be at least slightly
better.

Guenter

Powered by blists - more mailing lists