4.1 spidev module complains loudly about "buggy DT"

[pelwell]

spidev is the userspace interface to the SPI subsystem. It is (at least in some ways) analogous to i2c-dev.

The Raspberry Pi DTS files, in common with the upstream bcm2835 DTS's and those for many other platforms, use a compatible string of "spidev" to cause the spidev module to be loaded. A new commit in the upstream 4.1 branch (956b200) demonstrates the author's objection to this practice by causing the module to WARN when it detects that usage:

[   10.664420] spi spi0.1: setting up native-CS1 as GPIO 7
[   10.678260] spi spi0.0: setting up native-CS0 as GPIO 8
[   10.691016] spidev spi0.0: buggy DT: spidev listed directly in DT
[   10.705643] ------------[ cut here ]------------
[   10.717445] WARNING: CPU: 2 PID: 301 at drivers/spi/spidev.c:730 spidev_probe+0x180/0x1bc()
[   10.733972] Modules linked in: i2c_bcm2708(+) spi_bcm2835(+) w1_gpio wire cn uio_pdrv_genirq uio
[   10.750489] CPU: 2 PID: 301 Comm: modprobe Not tainted 4.1.1-v7+ #599
[   10.750507] Hardware name: BCM2709
[   10.750562] [<80018430>] (unwind_backtrace) from [<80013e0c>] (show_stack+0x20/0x24)
[   10.750588] [<80013e0c>] (show_stack) from [<805549e8>] (dump_stack+0x98/0xe0)
[   10.750619] [<805549e8>] (dump_stack) from [<80026a1c>] (warn_slowpath_common+0x8c/0xc8)
[   10.750641] [<80026a1c>] (warn_slowpath_common) from [<80026b14>] (warn_slowpath_null+0x2c/0x34)
[   10.750668] [<80026b14>] (warn_slowpath_null) from [<803bc79c>] (spidev_probe+0x180/0x1bc)
[   10.750689] [<803bc79c>] (spidev_probe) from [<803b9ea0>] (spi_drv_probe+0x48/0x4c)
[   10.750712] [<803b9ea0>] (spi_drv_probe) from [<80370394>] (really_probe+0x1b4/0x268)
[   10.750734] [<80370394>] (really_probe) from [<803704a8>] (__device_attach+0x60/0x64)
[   10.750754] [<803704a8>] (__device_attach) from [<8036e6f8>] (bus_for_each_drv+0x6c/0x9c)
[   10.750774] [<8036e6f8>] (bus_for_each_drv) from [<803701d4>] (device_attach+0x84/0x90)
[   10.750793] [<803701d4>] (device_attach) from [<8036f7d8>] (bus_probe_device+0x94/0xb8)
[   10.750812] [<8036f7d8>] (bus_probe_device) from [<8036d86c>] (device_add+0x3ac/0x53c)
[   10.750842] [<8036d86c>] (device_add) from [<803ba55c>] (spi_add_device+0x94/0x13c)
[   10.750866] [<803ba55c>] (spi_add_device) from [<803bbf6c>] (spi_register_master+0x42c/0x6e4)
[   10.750887] [<803bbf6c>] (spi_register_master) from [<803bc260>] (devm_spi_register_master+0x3c/0x78)
[   10.750923] [<803bc260>] (devm_spi_register_master) from [<7f03e3b8>] (bcm2835_spi_probe+0x174/0x220 [spi_bcm2835])
[   10.750960] [<7f03e3b8>] (bcm2835_spi_probe [spi_bcm2835]) from [<80371cd4>] (platform_drv_probe+0x3c/0x6c)
[   10.750981] [<80371cd4>] (platform_drv_probe) from [<80370394>] (really_probe+0x1b4/0x268)
[   10.751002] [<80370394>] (really_probe) from [<80370554>] (__driver_attach+0xa8/0xac)
[   10.751028] [<80370554>] (__driver_attach) from [<8036e630>] (bus_for_each_dev+0x74/0xa4)
[   10.751048] [<8036e630>] (bus_for_each_dev) from [<8036fdd0>] (driver_attach+0x28/0x30)
[   10.751072] [<8036fdd0>] (driver_attach) from [<8036fa68>] (bus_add_driver+0x154/0x1fc)
[   10.751095] [<8036fa68>] (bus_add_driver) from [<80370a1c>] (driver_register+0x88/0x108)
[   10.751114] [<80370a1c>] (driver_register) from [<80371c84>] (__platform_driver_register+0x58/0x6c)
[   10.751146] [<80371c84>] (__platform_driver_register) from [<7f041018>] (bcm2835_spi_driver_init+0x18/0x24 [spi_bcm2835])
[   10.751171] [<7f041018>] (bcm2835_spi_driver_init [spi_bcm2835]) from [<80009654>] (do_one_initcall+0x90/0x1ec)
[   10.751193] [<80009654>] (do_one_initcall) from [<80551838>] (do_init_module+0x6c/0x1bc)
[   10.751213] [<80551838>] (do_init_module) from [<80097b24>] (load_module+0x1ce8/0x1f30)
[   10.751240] [<80097b24>] (load_module) from [<80097e48>] (SyS_init_module+0xdc/0x134)
[   10.751260] [<80097e48>] (SyS_init_module) from [<8000f980>] (ret_fast_syscall+0x0/0x54)
[   10.751270] ---[ end trace 8d7e7c0750dd358b ]---

The commit message reads:

    spi: spidev: Warn loudly if instantiated from DT as "spidev"

    Since spidev is a detail of how Linux controls a device rather than a
    description of the hardware in the system we should never have a node
    described as "spidev" in DT, any SPI device could be a spidev so this
    is just not a useful description.

    In order to help prevent users from writing such device trees generate a
    warning if spidev is instantiated as a DT node without an ID in the match
    table.

    Signed-off-by: Mark Brown <broonie@kernel.org>

Following the analogy with i2c-dev, I can understand why this is frowned upon. Like i2c-dev, spidev does not talk directly to any hardware, but it is undoubtedly convenient to be able to load the module (with control over the maximum frequency) from the DT blob. And encouraging people to work around this by adding their specific devices to the list of compatible strings (i.e. making a generic module specific to a group of devices) seems bizarre.

There is currently only one device with a compatible string in the match table:

static const struct of_device_id spidev_dt_ids[] = {
       { .compatible = "rohm,dh2228fv" },
       {},
};

In other words, the only way to make an "approved" DT is currently to claim to be a DAC. This would probably break if that DAC ever got a dedicated driver module.

There are a few options that I can think of:

1. Rely on everybody who wants to use the user-space interface to control a device (which musn't have a driver) to add the name of that device to the list.
2. Create a Raspberry Pi-specific name for the spidev module ("rpi,spidev"), and add that to the list of compatible strings.
3. Add "spidev" to the list of compatible strings, which is enough to satisfy the letter of the test, but definitely not the spirit.
4. Revert the patch that adds the test.
5. Pretend that the Pi has one of those DACs on each SPI channel.

None of them are very palatable, but 3) seems the least disruptive.

@msperl: You've tried to raise this with the author - what are your thoughts?

[notro]

Even though I understand the rationale, I find it strange that this change was accepted. The mantra is to not break userpace, which I would argue that this change leads to. And doesn't it break the DT ABI also? There are in-kernel DT's that use spidev.
At least additional functionality should have been added so spi(dev) devices could be added from userspace like with i2c-dev.

I agree that 3) would be the best choice for now.

[msperl]

There have been a long 2 threads on the spi mailing list about that.
The only thing that came up was:

• add a different compatibility to spidev
• add a solution to load the module as a backup when no other compatible module is loaded
• make it work similar to i2c to change compatibility (via /sys)

Unfortunately no solution has materialized besides the recommendation of patching spidev -
I can not find that email by Mark now...

[msperl]

here one of those threads:
http://www.spinics.net/lists/linux-spi/msg03641.html

[pelwell]

@popcornmix Are you happy with option 3, at least as an interim measure? It's a one-line addition to the spidev driver:

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 92c909e..0535375 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -706,6 +706,7 @@ static struct class *spidev_class;
 #ifdef CONFIG_OF
 static const struct of_device_id spidev_dt_ids[] = {
        { .compatible = "rohm,dh2228fv" },
+       { .compatible = "spidev" },
        {},
 };
 MODULE_DEVICE_TABLE(of, spidev_dt_ids);

[popcornmix]

Okay by me.

[pelwell]

Pushed.