linux-kernel - Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to contain the hwmon suffix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140210165853.GD26997@lee--X1>
Date:	Mon, 10 Feb 2014 16:58:53 +0000
From:	Lee Jones <lee.jones@...aro.org>
To:	Jean Delvare <jdelvare@...e.de>
Cc:	Laszlo Papp <lpapp@....org>, linux-kernel@...r.kernel.org,
	Guenter Roeck <linux@...ck-us.net>, lm-sensors@...sensors.org
Subject: Re: [lm-sensors] [PATCH] hwmon: (max6650) Rename the device ids to
 contain the hwmon suffix

> > >  static const struct i2c_device_id max6650_id[] = {
> > > -	{ "max6650", 1 },
> > > -	{ "max6651", 4 },
> > > +	{ "max6650-hwmon", 1 },
> > > +	{ "max6651-hwmon", 4 },
> 
> No, this is not acceptable, sorry. This will change the name of the
> hwmon device as seen from user-space, breaking any configuration file
> referring to it. Additionally, dashes are explicitly forbidden in hwmon
> device names. And lastly this will break any explicit instantiation of
> theses devices (which is the only way, as the driver doesn't support
> device auto-detection), be it in the kernel itself or from user-space.
> 
> The change doesn't make sense anyway. If you move to the MFD framework,
> the core driver will be an I2C driver binding to the I2C device, and it
> will spawn the logical devices, presumably in the form of platform
> devices. That's what the current max6650 driver would have to bind to.
> Just renaming the device won't work, you also need to change the type.
> 
> If you want to turn this into an MFD driver, I believe you must first
> convert the hwmon part to register using
> devm_hwmon_device_register_with_groups(). This will dissociate the i2c
> device name from the hwmon device name and create a clean name-space
> for each function. Guenter, maybe you had a plan to do so already
> anyway?
> 
> That being said, going with MFD in this case seems quite overkill to
> me. MFD makes a lot of sense when each function has its own resources.
> As this isn't the case here, a single driver registering both an hwmon
> interface and a pinctrl interface would seem sufficient to me. But I
> think Guenter already discussed this in the past so I'll let him
> continue and decide.

I'll get you guys decide if you want to go the MFD route or not.

Either is okay with me, but if you do decide in favour, a name change
with the device type appended would be preferred. Else the core device
would have the same name as all of its children which would be quite
unworkable.

> > Might be worth taking the opportunity to swap out these magic numbers
> > now.
> 
> There's nothing magic about them, they tell the driver how many fans
> each device supports. If you don't pass them as driver_data you'll have
> to derive them from the device name in the probe function.

They're magic in that they're not easily identifiable. In the few
moments that I looked at the patch I assumed they were device
IDs. They should be clearly defined.

> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, max6650_id);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/