linux-kernel - Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

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 for Android: free password hash cracker in your pocket

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

Message-ID: <54115F6E.3010007@collabora.co.uk>
Date:	Thu, 11 Sep 2014 10:38:06 +0200
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Sjoerd Simons <sjoerd.simons@...labora.co.uk>,
	Lee Jones <lee.jones@...aro.org>,
	Wolfram Sang <wsa@...-dreams.de>
CC:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Nick Dyer <nick.dyer@...ev.co.uk>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH] Input: atmel_mxt_ts: Add of node type to the i2c table

Hello Lee,

On 09/11/2014 10:00 AM, Sjoerd Simons wrote:
>> > >  
>> > > -static const struct of_device_id mxt_of_match[] = {
>> > > -	{ .compatible = "atmel,maxtouch", },
>> > > -	{},
>> > > -};
>> > > -MODULE_DEVICE_TABLE(of, mxt_of_match);
>> > > -
>> > >  static const struct i2c_device_id mxt_id[] = {
>> > >  	{ "qt602240_ts", 0 },
>> > >  	{ "atmel_mxt_ts", 0 },
>> > >  	{ "atmel_mxt_tp", 0 },
>> > > +	{ "maxtouch", 0 },
>> > >  	{ "mXT224", 0 },
>> > >  	{ }
>> > >  };
>> > > @@ -2286,7 +2281,6 @@ static struct i2c_driver mxt_driver = {
>> > >  	.driver = {
>> > >  		.name	= "atmel_mxt_ts",
>> > >  		.owner	= THIS_MODULE,
>> > > -		.of_match_table = of_match_ptr(mxt_of_match),
>> > >  		.pm	= &mxt_pm_ops,
>> > >  	},
>> > >  	.probe		= mxt_probe,
>> > > 
>> > 
>> > I see that Lee is working to allow the I2C subsystem to not need an I2C ID
>> > table to match [0]. I'll let Lee to comment what the future plans are and if
>> > his series are going to solve your issue since I'm not that familiar with the
>> > I2C core.
>> 
>> It's wrong to expect DT to probe these devices without a compatible
>> string.  It does so at the moment, but this is a bi-product and not
>> the correct method.
> 
> Ok, which means removing the mxt_of_match table in this patch is wrong..
> I'll fix that for for a V2.
> 
> However that makes adding the "maxtouch" string to the i2c device table
> somewhat cumbersome as it only gets added in this case to ensure
> module-autoloading can happen as the modalias presented to userspace is
> going still going to be i2c:maxtouch.
> 
> Tbh, the bigger problem this is pointing out is that for I2C devices
> with only an OF compability tring module auto-loading is broken...
> 

To expand on what Sjoerd already said and just to be sure everyone is on the
same page.

The problem is that right now the driver reports the following modalias:

# cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
i2c:maxtouch

but if you look at the module information, that is not a valid alias:

# modinfo atmel_mxt_ts | grep alias
alias:          i2c:mXT224
alias:          i2c:atmel_mxt_tp
alias:          i2c:atmel_mxt_ts
alias:          i2c:qt602240_ts
alias:          of:N*T*Catmel,maxtouch*

which means that udev/kmod can't load the module automatically based on the
alias information.

The aliases are filled by both MODULE_DEVICE_TABLE(i2c, mxt_id) and
MODULE_DEVICE_TABLE(of, mxt_of_match) so after Sjoerd's patch:

# cat /sys/class/i2c-adapter/i2c-8/8-004b/modalias
i2c:maxtouch

# modinfo atmel_mxt_ts | grep alias
alias:          i2c:mXT224
alias:          i2c:maxtouch
alias:          i2c:atmel_mxt_tp
alias:          i2c:atmel_mxt_ts
alias:          i2c:qt602240_ts

which matches the reported uevent so the module will be auto-loaded.

This is because the I2C subsystem hardcodes i2c:<client->name>, if you look at
drivers/i2c/i2c-core.c:

/* uevent helps with hotplug: modprobe -q $(MODALIAS) */
static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
{
...
        if (add_uevent_var(env, "MODALIAS=%s%s",
                           I2C_MODULE_PREFIX, client->name))
...
}

I've looked at Lee's series and AFAICT that remains the same so I second
Sjoerd that module auto-loading will continue to be broken.

Best regards,
Javier
--
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/