linux-kernel - Re: spi: OF module autoloading is still broken

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] [day] [month] [year] [list]
Message-ID: <564DC4D6.2010506@osg.samsung.com>
Date:	Thu, 19 Nov 2015 09:47:18 -0300
From:	Javier Martinez Canillas <javier@....samsung.com>
To:	Brian Norris <computersforpeace@...il.com>
Cc:	Mark Brown <broonie@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	linux-mtd@...ts.infradead.org,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org
Subject: Re: spi: OF module autoloading is still broken

Hello,

On 11/18/2015 05:07 PM, Javier Martinez Canillas wrote:
> Hello Brian and Mark,
> 
> On 11/16/2015 06:32 PM, Javier Martinez Canillas wrote:
>> On 11/16/2015 05:47 PM, Brian Norris wrote:
>>> On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote:
>>>> On 11/16/2015 04:24 PM, Brian Norris wrote:
> 
> [snip]
> 
>>>>> I don't think you have problems only with bad FDTs. I think you have a
>>>>> problem with perfectly valid DTs.
>>>>>
>>>>
>>>> Agreed, I wonder if spi_uevent() shouldn't be changed to report both the
>>>> OF and old SPI modaliases to avoid breaking a lot of drivers but at the
>>>> same time allowing DT-only drivers to not need an SPI ID table.
>>>
>>> That's the solution I imagined, though I haven't tested it yet. I don't
>>> see much precedent for reporting multiple modaliases, so there could be
>>> some kind of ABI issues around that too.
>>>
>>
>> Ok, I'm glad that we agree. This definitely needs to be discussed with more
>> people. I'll re-spin my patch and post a v2 reporting multiple modaliases
>> tomorrow after testing.
>>
> 
> So I had some time today to test this approach but unfortunately it seems
> this workaround will not be possible because AFAICT kmod only takes into
> account the last MODALIAS reported as a uevent. I still have to check the
> kmod source code but that's my conclusion from trying to report both aliases.
>

I dig a little more on this and is udev and not kmod that can't cope with
more than one MODALIAS, kmod just use the alias that udev tells it to use.

1) OF modalias reported after SPI modalias
------------------------------------------

cat /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0/uevent
DRIVER=cros-ec-spi
OF_NAME=cros-ec
OF_FULLNAME=/spi@...40000/cros-ec@0
OF_COMPATIBLE_0=google,cros-ec-spi
OF_COMPATIBLE_N=1
MODALIAS=spi:cros-ec-spi
MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi

udevadm test /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0
ACTION=add
DEVPATH=/devices/platform/12d40000.spi/spi_master/spi2/spi2.0
DRIVER=cros-ec-spi
MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi
OF_COMPATIBLE_0=google,cros-ec-spi
OF_COMPATIBLE_N=1
OF_FULLNAME=/spi@...40000/cros-ec@0
OF_NAME=cros-ec
SUBSYSTEM=spi
USEC_INITIALIZED=52732787
run: 'kmod load of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi'

2) SPI modalias reported after OF modalias
------------------------------------------

cat /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0/uevent
DRIVER=cros-ec-spi
OF_NAME=cros-ec
OF_FULLNAME=/spi@...40000/cros-ec@0
OF_COMPATIBLE_0=google,cros-ec-spi
OF_COMPATIBLE_N=1
MODALIAS=of:Ncros-ecT<NULL>Cgoogle,cros-ec-spi
MODALIAS=spi:cros-ec-spi

udevadm test /sys/devices/platform/12d40000.spi/spi_master/spi2/spi2.0
ACTION=add
DEVPATH=/devices/platform/12d40000.spi/spi_master/spi2/spi2.0
DRIVER=cros-ec-spi
MODALIAS=spi:cros-ec-spi
OF_COMPATIBLE_0=google,cros-ec-spi
OF_COMPATIBLE_N=1
OF_FULLNAME=/spi@...40000/cros-ec@0
OF_NAME=cros-ec
SUBSYSTEM=spi
USEC_INITIALIZED=288316488
run: 'kmod load spi:cros-ec-spi'

So as you can see: 'kmod load $modalias' is only called for the last modalias.
 
> 
> IOW, even when is possible to report more than one MODALIAS, user-space seems
> to only use the last one reported so using both don't work.
> 
> Of course kmod can be changed to check for more than one MODALIAS but since
> the kernel should not break old user-space, the fact that a single MODALIAS
> was reported seems to be an ABI now due how is used.
>

So I think our options are:

a) Not change spi_uevent() to report an OF modalias and make a requirement
to have a spi_device_id table even for OF-only to have autoload working.

Regardless of the option, SPI ID tables should be present in drivers so
these are supported in non-OF platforms as Mark said.

b) Make sure that all OF drivers have a complete OF table with all entries
in the SPI ID table before spi_uevent() is modified to report OF modaliases.

My preference would be b) so the same table (OF or SPI ID) can be used for
alias filling that match reported modalias and driver to device matching
and will also be aligned with what other subsystems do.

I'll check my script to see why the drivers/mtd/devices/m25p80.c was not
found, likely because the entries in the spi_device_id table weren't in
the DT binding doc before.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
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/