Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 08:33:12PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 19:19, Mark Brown  wrote:

> > It's a string that's exposed to userspace (via /proc/devices if nothing
> > else).

> Yes, the change can be detected from userspace. The question is
> more like 'Is there any sane reason anyone would rely on seeing
> that exact string for something?'

People using it to create device files at runtime or detect board
variants would be the main ones.

> BTW the classes are also exported through /sys so the spidev devices
> would look differently there also.

Yup.

> That does not make device-specific hacks go away. Initially the user
> will do only as much as it takes to get the device working right now
> and that does not include writing devicetree description of the device
> when it is used with userspace driver.

Depends, especially if you see some long term use for your code it's
common to start with the enumeration part.  It's not there currently so
people can't do that but part of getting the good practice out there is
to make it the path of least resistance.


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 08:33:12PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 19:19, Mark Brown  wrote:

> > It's a string that's exposed to userspace (via /proc/devices if nothing
> > else).

> Yes, the change can be detected from userspace. The question is
> more like 'Is there any sane reason anyone would rely on seeing
> that exact string for something?'

People using it to create device files at runtime or detect board
variants would be the main ones.

> BTW the classes are also exported through /sys so the spidev devices
> would look differently there also.

Yup.

> That does not make device-specific hacks go away. Initially the user
> will do only as much as it takes to get the device working right now
> and that does not include writing devicetree description of the device
> when it is used with userspace driver.

Depends, especially if you see some long term use for your code it's
common to start with the enumeration part.  It's not there currently so
people can't do that but part of getting the good practice out there is
to make it the path of least resistance.


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

On 19 July 2016 at 19:19, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 05:32:24PM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 14:44, Mark Brown  wrote:
>
>> >> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
>> >> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);
>
>> > also looks like a needless ABI change
>
>> ABI change to what?
>
>> The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.
>
>> What parts of the kernel or userspace should depend on spidev
>> devices belonging to class spi?
>
> It's a string that's exposed to userspace (via /proc/devices if nothing
> else).

Yes, the change can be detected from userspace. The question is
more like 'Is there any sane reason anyone would rely on seeing
that exact string for something?'

BTW the classes are also exported through /sys so the spidev devices
would look differently there also.

>
>> > If userspace is managing to figure out how to control the device then
>> > providing a description of the hardware is clearly within the bounds of
>> > possibility and there is no need to fake anything.
>
>> However. maintaining the hardware description in multiple places is
>> redundant and error-prone. Since the userspace somehow managed to
>> figure it out on legacy kernels without devicetree it does not need
>> the information in devicetree. Users of such software will not want the
>> hardware description in devicetree and if forced to provide it will stub it 
>> out.
>
> Userspace is just as capable of looking at the DT as anything else, I'd
> expect that where this is a viable way of deploying things people would
> be writing udev/systemd magic to fire up userspace drivers automatically
> when they see suitable hardware.  The only way this has ever worked in
> the past has been with system specific hacks which lead to fragile,
> non-portable userpaces that are hard to upgrade.  If we're trying to say
> this is an interface that's there for all devices then we shouldn't be
> defining it in a way that makes it difficult to automatically configure.

Oh sure. Once a device-specific hack turns out to be something
desirable to maintain over long time and deploy on variety of devices we
have infrastructure to automate that.

That does not make device-specific hacks go away. Initially the user
will do only as much as it takes to get the device working right now
and that does not include writing devicetree description of the device
when it is used with userspace driver.

>
>> Also world is not all devicetree so userspace applications should be free
>> to consult devicetree information if present or use whatever other means
>> at their disposal to determine what hardware they are dealing with.
>> When portability is a concern consulting devicetree may be a secondary
>> source at best.
>
> ACPI has similar facilities (it's got a direct translation of DT in it
> these days so most things with a DT binding also have an ACPI one for
> free).

There is even a patch floating around for adding empty DT and some proxy
nodes for existing hardware so dynamically connected devices can be
configured with devicetree overlays on boards without devicetree.

So you can have devicetree description of the hardware. It's just sometimes
not useful.

Thanks

Michal

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

On 19 July 2016 at 19:19, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 05:32:24PM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 14:44, Mark Brown  wrote:
>
>> >> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
>> >> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);
>
>> > also looks like a needless ABI change
>
>> ABI change to what?
>
>> The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.
>
>> What parts of the kernel or userspace should depend on spidev
>> devices belonging to class spi?
>
> It's a string that's exposed to userspace (via /proc/devices if nothing
> else).

Yes, the change can be detected from userspace. The question is
more like 'Is there any sane reason anyone would rely on seeing
that exact string for something?'

BTW the classes are also exported through /sys so the spidev devices
would look differently there also.

>
>> > If userspace is managing to figure out how to control the device then
>> > providing a description of the hardware is clearly within the bounds of
>> > possibility and there is no need to fake anything.
>
>> However. maintaining the hardware description in multiple places is
>> redundant and error-prone. Since the userspace somehow managed to
>> figure it out on legacy kernels without devicetree it does not need
>> the information in devicetree. Users of such software will not want the
>> hardware description in devicetree and if forced to provide it will stub it 
>> out.
>
> Userspace is just as capable of looking at the DT as anything else, I'd
> expect that where this is a viable way of deploying things people would
> be writing udev/systemd magic to fire up userspace drivers automatically
> when they see suitable hardware.  The only way this has ever worked in
> the past has been with system specific hacks which lead to fragile,
> non-portable userpaces that are hard to upgrade.  If we're trying to say
> this is an interface that's there for all devices then we shouldn't be
> defining it in a way that makes it difficult to automatically configure.

Oh sure. Once a device-specific hack turns out to be something
desirable to maintain over long time and deploy on variety of devices we
have infrastructure to automate that.

That does not make device-specific hacks go away. Initially the user
will do only as much as it takes to get the device working right now
and that does not include writing devicetree description of the device
when it is used with userspace driver.

>
>> Also world is not all devicetree so userspace applications should be free
>> to consult devicetree information if present or use whatever other means
>> at their disposal to determine what hardware they are dealing with.
>> When portability is a concern consulting devicetree may be a secondary
>> source at best.
>
> ACPI has similar facilities (it's got a direct translation of DT in it
> these days so most things with a DT binding also have an ACPI one for
> free).

There is even a patch floating around for adding empty DT and some proxy
nodes for existing hardware so dynamically connected devices can be
configured with devicetree overlays on boards without devicetree.

So you can have devicetree description of the hardware. It's just sometimes
not useful.

Thanks

Michal

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 05:32:24PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 14:44, Mark Brown  wrote:

> >> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
> >> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);

> > also looks like a needless ABI change

> ABI change to what?

> The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.

> What parts of the kernel or userspace should depend on spidev
> devices belonging to class spi?

It's a string that's exposed to userspace (via /proc/devices if nothing
else).

> > If userspace is managing to figure out how to control the device then
> > providing a description of the hardware is clearly within the bounds of
> > possibility and there is no need to fake anything.

> However. maintaining the hardware description in multiple places is
> redundant and error-prone. Since the userspace somehow managed to
> figure it out on legacy kernels without devicetree it does not need
> the information in devicetree. Users of such software will not want the
> hardware description in devicetree and if forced to provide it will stub it 
> out.

Userspace is just as capable of looking at the DT as anything else, I'd
expect that where this is a viable way of deploying things people would
be writing udev/systemd magic to fire up userspace drivers automatically
when they see suitable hardware.  The only way this has ever worked in
the past has been with system specific hacks which lead to fragile,
non-portable userpaces that are hard to upgrade.  If we're trying to say
this is an interface that's there for all devices then we shouldn't be
defining it in a way that makes it difficult to automatically configure.

> Also world is not all devicetree so userspace applications should be free
> to consult devicetree information if present or use whatever other means
> at their disposal to determine what hardware they are dealing with.
> When portability is a concern consulting devicetree may be a secondary
> source at best.

ACPI has similar facilities (it's got a direct translation of DT in it
these days so most things with a DT binding also have an ACPI one for
free).


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 05:32:24PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 14:44, Mark Brown  wrote:

> >> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
> >> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);

> > also looks like a needless ABI change

> ABI change to what?

> The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.

> What parts of the kernel or userspace should depend on spidev
> devices belonging to class spi?

It's a string that's exposed to userspace (via /proc/devices if nothing
else).

> > If userspace is managing to figure out how to control the device then
> > providing a description of the hardware is clearly within the bounds of
> > possibility and there is no need to fake anything.

> However. maintaining the hardware description in multiple places is
> redundant and error-prone. Since the userspace somehow managed to
> figure it out on legacy kernels without devicetree it does not need
> the information in devicetree. Users of such software will not want the
> hardware description in devicetree and if forced to provide it will stub it 
> out.

Userspace is just as capable of looking at the DT as anything else, I'd
expect that where this is a viable way of deploying things people would
be writing udev/systemd magic to fire up userspace drivers automatically
when they see suitable hardware.  The only way this has ever worked in
the past has been with system specific hacks which lead to fragile,
non-portable userpaces that are hard to upgrade.  If we're trying to say
this is an interface that's there for all devices then we shouldn't be
defining it in a way that makes it difficult to automatically configure.

> Also world is not all devicetree so userspace applications should be free
> to consult devicetree information if present or use whatever other means
> at their disposal to determine what hardware they are dealing with.
> When portability is a concern consulting devicetree may be a secondary
> source at best.

ACPI has similar facilities (it's got a direct translation of DT in it
these days so most things with a DT binding also have an ACPI one for
free).


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

On 19 July 2016 at 14:44, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 01:17:52PM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 00:59, Mark Brown  wrote:
>
>> Looking into SPI more this whole point is mostly moot, anyway. Most of the
>> time the unused CS lines will not be multiplexed to the SPI controller so
>> even if the SPI master tries to use them it does nothing. Also most of the
>> time all the CS lines can be exported as GPIO regardless of their use for 
>> SPI.
>> And set to any possible level using the GPIO userspace API.
>
> Your system is not the entire world, Linux has to run on other systems
> too.  There are actual devices out there with inflexible pinmuxing, we
> can't just ignore them.

And they are not ignored.

>
>> In order to reduce the amount of objects and management I appended
>> the link object at the end of struct spi_device.
>
>> Is there a better way?
>
> That seems mostly fine, the problem is one of code review - because the
> patch does many logically distinct things in a single patch it is much
> harder to review than if it were split out.  There's a lot of mechanical
> refactoring which obscures the several more substantive changes that are
> being made at the same time and since most of those substantive changes
> are explicitly described at all it's hard to tell if they're intended or
> doing what is expected.
>
>> > I am getting completely fed up of saying this, it's not OK to just
>> > expose pins to userspace when we have no idea what they are connected
>> > to.
>
>> I am getting fed up with this argument.
>
>> First, this patchset only exposes to userspace pins that are configured as 
>> SPI
>> slaves in devicetree.
>
> Are you *absolutely* positive your changes won't do anything on non-DT
> systems?  That definitely doesn't appear to be the case (and shouldn't
> be, not all the world is DT), but now I look in more detail what it is
> doing is exposing spidev for each explicitly described slave which is
> substantially more safe and does address the main problem with
> undescribed devices.
>
> Please split this up so it can be reviewed, providing clear and specific
> descriptions of each individual change (as covered in
> SubmittingPatches).

OK, splitting this into multiple incremental patches should work better.

>
> This:
>
>> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
>> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);
>
> also looks like a needless ABI change

ABI change to what?

The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.

What parts of the kernel or userspace should depend on spidev
devices belonging to class spi?

Changing this is deliberate so SPI devices can be enumerated walking
the bus without seeing the spidev devices. Or spidev devices can be
enumerated walking the spidev class. No need to maintain a list.

> and the removal of the locking on
> minor device allocation seems concerning and at least needs to be
> covered in the changelog.

This particular part should be covered by the SPI device list lock
because the attach/detach function should be only called with
the lock held.

It's certainly worth documenting as a separate change if it's still preserved
once spidev is again modularized.

>
>> The fact that devicetree does not allow talking to a SPI device from 
>> userspace
>> without telling kernel what kind of protocol userspace is using is a 
>> regression
>> from using board files. The kernel has no business knowing that. When you
>> use a userspace driver it's userspace's job to manage protocols and devices.
>
> The system as a whole, including both the kernel and userspace, has
> every business knowing what the hardware is so it can understand how to
> control it.  Userspace is no more able to discover what the magic
> undescribed bits of hardware are automatically than the kernel is
> without help from a hardware description, the DT isn't just something
> for the kernel once you have userspace drivers.
>
> Directly enumerating spidev wasn't an especially great idea for board
> files either, it was just an expedient hack that worked when the
> hardware description was entirely in kernel and much easier to just go
> and change but now we have an external ABI for hardware description and
> we have to expect that people will use it as such.
>
>> The purpose of this patchset is for the userspace applications to continue 
>> doing
>> so with devicetree based systems without the need to fabricate fake hardware
>> description. And the hardware description will necessarily be fake in the 
>> case
>> when it is in fact userspace implementing and managing the drivers.
>
> If userspace is managing to figure out how to control the device then
> providing a description of the hardware is clearly within the bounds of
> possibility and there is no need to fake anything.

However. maintaining the hardware description in multiple places is
redundant 

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

On 19 July 2016 at 14:44, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 01:17:52PM +0200, Michal Suchanek wrote:
>> On 19 July 2016 at 00:59, Mark Brown  wrote:
>
>> Looking into SPI more this whole point is mostly moot, anyway. Most of the
>> time the unused CS lines will not be multiplexed to the SPI controller so
>> even if the SPI master tries to use them it does nothing. Also most of the
>> time all the CS lines can be exported as GPIO regardless of their use for 
>> SPI.
>> And set to any possible level using the GPIO userspace API.
>
> Your system is not the entire world, Linux has to run on other systems
> too.  There are actual devices out there with inflexible pinmuxing, we
> can't just ignore them.

And they are not ignored.

>
>> In order to reduce the amount of objects and management I appended
>> the link object at the end of struct spi_device.
>
>> Is there a better way?
>
> That seems mostly fine, the problem is one of code review - because the
> patch does many logically distinct things in a single patch it is much
> harder to review than if it were split out.  There's a lot of mechanical
> refactoring which obscures the several more substantive changes that are
> being made at the same time and since most of those substantive changes
> are explicitly described at all it's hard to tell if they're intended or
> doing what is expected.
>
>> > I am getting completely fed up of saying this, it's not OK to just
>> > expose pins to userspace when we have no idea what they are connected
>> > to.
>
>> I am getting fed up with this argument.
>
>> First, this patchset only exposes to userspace pins that are configured as 
>> SPI
>> slaves in devicetree.
>
> Are you *absolutely* positive your changes won't do anything on non-DT
> systems?  That definitely doesn't appear to be the case (and shouldn't
> be, not all the world is DT), but now I look in more detail what it is
> doing is exposing spidev for each explicitly described slave which is
> substantially more safe and does address the main problem with
> undescribed devices.
>
> Please split this up so it can be reviewed, providing clear and specific
> descriptions of each individual change (as covered in
> SubmittingPatches).

OK, splitting this into multiple incremental patches should work better.

>
> This:
>
>> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
>> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);
>
> also looks like a needless ABI change

ABI change to what?

The spidev ABI should be the IOCTLs on /dev/spidev*. These are preserved.

What parts of the kernel or userspace should depend on spidev
devices belonging to class spi?

Changing this is deliberate so SPI devices can be enumerated walking
the bus without seeing the spidev devices. Or spidev devices can be
enumerated walking the spidev class. No need to maintain a list.

> and the removal of the locking on
> minor device allocation seems concerning and at least needs to be
> covered in the changelog.

This particular part should be covered by the SPI device list lock
because the attach/detach function should be only called with
the lock held.

It's certainly worth documenting as a separate change if it's still preserved
once spidev is again modularized.

>
>> The fact that devicetree does not allow talking to a SPI device from 
>> userspace
>> without telling kernel what kind of protocol userspace is using is a 
>> regression
>> from using board files. The kernel has no business knowing that. When you
>> use a userspace driver it's userspace's job to manage protocols and devices.
>
> The system as a whole, including both the kernel and userspace, has
> every business knowing what the hardware is so it can understand how to
> control it.  Userspace is no more able to discover what the magic
> undescribed bits of hardware are automatically than the kernel is
> without help from a hardware description, the DT isn't just something
> for the kernel once you have userspace drivers.
>
> Directly enumerating spidev wasn't an especially great idea for board
> files either, it was just an expedient hack that worked when the
> hardware description was entirely in kernel and much easier to just go
> and change but now we have an external ABI for hardware description and
> we have to expect that people will use it as such.
>
>> The purpose of this patchset is for the userspace applications to continue 
>> doing
>> so with devicetree based systems without the need to fabricate fake hardware
>> description. And the hardware description will necessarily be fake in the 
>> case
>> when it is in fact userspace implementing and managing the drivers.
>
> If userspace is managing to figure out how to control the device then
> providing a description of the hardware is clearly within the bounds of
> possibility and there is no need to fake anything.

However. maintaining the hardware description in multiple places is
redundant and error-prone. Since the userspace 

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 01:17:52PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 00:59, Mark Brown  wrote:

> Looking into SPI more this whole point is mostly moot, anyway. Most of the
> time the unused CS lines will not be multiplexed to the SPI controller so
> even if the SPI master tries to use them it does nothing. Also most of the
> time all the CS lines can be exported as GPIO regardless of their use for SPI.
> And set to any possible level using the GPIO userspace API.

Your system is not the entire world, Linux has to run on other systems
too.  There are actual devices out there with inflexible pinmuxing, we
can't just ignore them.

> In order to reduce the amount of objects and management I appended
> the link object at the end of struct spi_device.

> Is there a better way?

That seems mostly fine, the problem is one of code review - because the
patch does many logically distinct things in a single patch it is much
harder to review than if it were split out.  There's a lot of mechanical
refactoring which obscures the several more substantive changes that are
being made at the same time and since most of those substantive changes
are explicitly described at all it's hard to tell if they're intended or
doing what is expected.

> > I am getting completely fed up of saying this, it's not OK to just
> > expose pins to userspace when we have no idea what they are connected
> > to.

> I am getting fed up with this argument.

> First, this patchset only exposes to userspace pins that are configured as SPI
> slaves in devicetree.

Are you *absolutely* positive your changes won't do anything on non-DT
systems?  That definitely doesn't appear to be the case (and shouldn't
be, not all the world is DT), but now I look in more detail what it is
doing is exposing spidev for each explicitly described slave which is
substantially more safe and does address the main problem with
undescribed devices.

Please split this up so it can be reviewed, providing clear and specific
descriptions of each individual change (as covered in
SubmittingPatches).

This:

> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);

also looks like a needless ABI change and the removal of the locking on
minor device allocation seems concerning and at least needs to be
covered in the changelog.

> The fact that devicetree does not allow talking to a SPI device from userspace
> without telling kernel what kind of protocol userspace is using is a 
> regression
> from using board files. The kernel has no business knowing that. When you
> use a userspace driver it's userspace's job to manage protocols and devices.

The system as a whole, including both the kernel and userspace, has
every business knowing what the hardware is so it can understand how to
control it.  Userspace is no more able to discover what the magic
undescribed bits of hardware are automatically than the kernel is
without help from a hardware description, the DT isn't just something
for the kernel once you have userspace drivers.  

Directly enumerating spidev wasn't an especially great idea for board
files either, it was just an expedient hack that worked when the
hardware description was entirely in kernel and much easier to just go
and change but now we have an external ABI for hardware description and
we have to expect that people will use it as such.

> The purpose of this patchset is for the userspace applications to continue 
> doing
> so with devicetree based systems without the need to fabricate fake hardware
> description. And the hardware description will necessarily be fake in the case
> when it is in fact userspace implementing and managing the drivers.

If userspace is managing to figure out how to control the device then
providing a description of the hardware is clearly within the bounds of
possibility and there is no need to fake anything.


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 01:17:52PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 00:59, Mark Brown  wrote:

> Looking into SPI more this whole point is mostly moot, anyway. Most of the
> time the unused CS lines will not be multiplexed to the SPI controller so
> even if the SPI master tries to use them it does nothing. Also most of the
> time all the CS lines can be exported as GPIO regardless of their use for SPI.
> And set to any possible level using the GPIO userspace API.

Your system is not the entire world, Linux has to run on other systems
too.  There are actual devices out there with inflexible pinmuxing, we
can't just ignore them.

> In order to reduce the amount of objects and management I appended
> the link object at the end of struct spi_device.

> Is there a better way?

That seems mostly fine, the problem is one of code review - because the
patch does many logically distinct things in a single patch it is much
harder to review than if it were split out.  There's a lot of mechanical
refactoring which obscures the several more substantive changes that are
being made at the same time and since most of those substantive changes
are explicitly described at all it's hard to tell if they're intended or
doing what is expected.

> > I am getting completely fed up of saying this, it's not OK to just
> > expose pins to userspace when we have no idea what they are connected
> > to.

> I am getting fed up with this argument.

> First, this patchset only exposes to userspace pins that are configured as SPI
> slaves in devicetree.

Are you *absolutely* positive your changes won't do anything on non-DT
systems?  That definitely doesn't appear to be the case (and shouldn't
be, not all the world is DT), but now I look in more detail what it is
doing is exposing spidev for each explicitly described slave which is
substantially more safe and does address the main problem with
undescribed devices.

Please split this up so it can be reviewed, providing clear and specific
descriptions of each individual change (as covered in
SubmittingPatches).

This:

> -   status = register_chrdev(SPIDEV_MAJOR, "spi", _fops);
> +   status = register_chrdev(SPIDEV_MAJOR, "spidev", _fops);

also looks like a needless ABI change and the removal of the locking on
minor device allocation seems concerning and at least needs to be
covered in the changelog.

> The fact that devicetree does not allow talking to a SPI device from userspace
> without telling kernel what kind of protocol userspace is using is a 
> regression
> from using board files. The kernel has no business knowing that. When you
> use a userspace driver it's userspace's job to manage protocols and devices.

The system as a whole, including both the kernel and userspace, has
every business knowing what the hardware is so it can understand how to
control it.  Userspace is no more able to discover what the magic
undescribed bits of hardware are automatically than the kernel is
without help from a hardware description, the DT isn't just something
for the kernel once you have userspace drivers.  

Directly enumerating spidev wasn't an especially great idea for board
files either, it was just an expedient hack that worked when the
hardware description was entirely in kernel and much easier to just go
and change but now we have an external ABI for hardware description and
we have to expect that people will use it as such.

> The purpose of this patchset is for the userspace applications to continue 
> doing
> so with devicetree based systems without the need to fabricate fake hardware
> description. And the hardware description will necessarily be fake in the case
> when it is in fact userspace implementing and managing the drivers.

If userspace is managing to figure out how to control the device then
providing a description of the hardware is clearly within the bounds of
possibility and there is no need to fake anything.


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

Hello,

On 19 July 2016 at 00:59, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 12:35:40AM +0200, Michal Suchanek wrote:
>
>>  config SPI_SPIDEV
>> - tristate "User mode SPI device driver support"
>> + bool "User mode SPI device driver support"
>
> This is a step back, it would require spidev to be built in.

That's a technical problem so it can be addressed.

>
>> - spin_lock_irq(>spi_lock);
>> - spi = spi_dev_get(spidev->spi);
>> - spin_unlock_irq(>spi_lock);
>> + spi = filp->private_data;
>> + spi = spi_dev_get(spi);
>
> All this refactoring to move spidev about should've been a separate
> patch, it's hard to find the actual content in here.

And re-modularizing will probably need to move thing back, mostly.

Regarding this change

spidev uses device_create to allocate spidev character devices so
these are not part of some struct spidev_device but live separately
which gives needlessly more objects to manage

- the spi device
- the spidev character device
- a link object that points to both spi and spidev and is manually
  managed by the spidev driver

In order to reduce the amount of objects and management I appended
the link object at the end of struct spi_device.

Is there a better way?

>
>> - mutex_lock(_list_lock);
>> + dev = bus_find_device(_bus_type, NULL, (void *) inode->i_rdev,
>> +   spidev_devt_match);
>
> ...
>
>> - dev_dbg(>spi->dev, "open/ENOMEM\n");
>> + spi = to_spi_device(dev);
>> +
>> + mutex_lock(>buf_lock);
>> + spin_lock_irq(>spidev_lock);
>> + spi->spidev_users++;
>> + spin_unlock_irq(>spidev_lock);
>
> This is broken, it will unconditionally create a spidev for any chip
> select regardless of if there's any actual hardware there and (even more
> importantly) regardless if that hardware is actually a SPI device.  This
> is not safe, especially given some of the ideas people seem to have for
> userspaces.

This is as safe as it gets. For the device probe to happen somebody must
have configured the CS as spi slave device somewhere. What this patchset
accomplishes is creating an userspace interface for accessing SPI bus using
the CS regardless of the probe result (at least for devicetree nodes).

In particular it does not create a spidev device for CS lines which are unused.

Looking into SPI more this whole point is mostly moot, anyway. Most of the
time the unused CS lines will not be multiplexed to the SPI controller so
even if the SPI master tries to use them it does nothing. Also most of the
time all the CS lines can be exported as GPIO regardless of their use for SPI.
And set to any possible level using the GPIO userspace API.

>
> I am getting completely fed up of saying this, it's not OK to just
> expose pins to userspace when we have no idea what they are connected
> to.

I am getting fed up with this argument.

First, this patchset only exposes to userspace pins that are configured as SPI
slaves in devicetree.

It does so even in the cases when the driver for the device in question is not
available in the kernel (because it is not loaded or excluded completely) or
in the case you do not specify which SPI slave is connected to those pins.
This is the only change.

So in addition to saying 'load driver XY with parameters Z=BJ' this allows
saying 'there is possibly a SPI device and I may want to talk to it with an
userspace application'. With overlays this can be later amended to 'load
driver ZY with parameters X=XN' or whatever.

The fact that devicetree does not allow talking to a SPI device from userspace
without telling kernel what kind of protocol userspace is using is a regression
from using board files. The kernel has no business knowing that. When you
use a userspace driver it's userspace's job to manage protocols and devices.
The purpose of this patchset is for the userspace applications to continue doing
so with devicetree based systems without the need to fabricate fake hardware
description. And the hardware description will necessarily be fake in the case
when it is in fact userspace implementing and managing the drivers.

It's not to say that userspace drivers that duplicate kernel drivers cannot
examine the devicetree for configuration data. That's up to the userspace
driver writer to decide, however.

Thanks

Michal

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Michal Suchanek]

Hello,

On 19 July 2016 at 00:59, Mark Brown  wrote:
> On Tue, Jul 19, 2016 at 12:35:40AM +0200, Michal Suchanek wrote:
>
>>  config SPI_SPIDEV
>> - tristate "User mode SPI device driver support"
>> + bool "User mode SPI device driver support"
>
> This is a step back, it would require spidev to be built in.

That's a technical problem so it can be addressed.

>
>> - spin_lock_irq(>spi_lock);
>> - spi = spi_dev_get(spidev->spi);
>> - spin_unlock_irq(>spi_lock);
>> + spi = filp->private_data;
>> + spi = spi_dev_get(spi);
>
> All this refactoring to move spidev about should've been a separate
> patch, it's hard to find the actual content in here.

And re-modularizing will probably need to move thing back, mostly.

Regarding this change

spidev uses device_create to allocate spidev character devices so
these are not part of some struct spidev_device but live separately
which gives needlessly more objects to manage

- the spi device
- the spidev character device
- a link object that points to both spi and spidev and is manually
  managed by the spidev driver

In order to reduce the amount of objects and management I appended
the link object at the end of struct spi_device.

Is there a better way?

>
>> - mutex_lock(_list_lock);
>> + dev = bus_find_device(_bus_type, NULL, (void *) inode->i_rdev,
>> +   spidev_devt_match);
>
> ...
>
>> - dev_dbg(>spi->dev, "open/ENOMEM\n");
>> + spi = to_spi_device(dev);
>> +
>> + mutex_lock(>buf_lock);
>> + spin_lock_irq(>spidev_lock);
>> + spi->spidev_users++;
>> + spin_unlock_irq(>spidev_lock);
>
> This is broken, it will unconditionally create a spidev for any chip
> select regardless of if there's any actual hardware there and (even more
> importantly) regardless if that hardware is actually a SPI device.  This
> is not safe, especially given some of the ideas people seem to have for
> userspaces.

This is as safe as it gets. For the device probe to happen somebody must
have configured the CS as spi slave device somewhere. What this patchset
accomplishes is creating an userspace interface for accessing SPI bus using
the CS regardless of the probe result (at least for devicetree nodes).

In particular it does not create a spidev device for CS lines which are unused.

Looking into SPI more this whole point is mostly moot, anyway. Most of the
time the unused CS lines will not be multiplexed to the SPI controller so
even if the SPI master tries to use them it does nothing. Also most of the
time all the CS lines can be exported as GPIO regardless of their use for SPI.
And set to any possible level using the GPIO userspace API.

>
> I am getting completely fed up of saying this, it's not OK to just
> expose pins to userspace when we have no idea what they are connected
> to.

I am getting fed up with this argument.

First, this patchset only exposes to userspace pins that are configured as SPI
slaves in devicetree.

It does so even in the cases when the driver for the device in question is not
available in the kernel (because it is not loaded or excluded completely) or
in the case you do not specify which SPI slave is connected to those pins.
This is the only change.

So in addition to saying 'load driver XY with parameters Z=BJ' this allows
saying 'there is possibly a SPI device and I may want to talk to it with an
userspace application'. With overlays this can be later amended to 'load
driver ZY with parameters X=XN' or whatever.

The fact that devicetree does not allow talking to a SPI device from userspace
without telling kernel what kind of protocol userspace is using is a regression
from using board files. The kernel has no business knowing that. When you
use a userspace driver it's userspace's job to manage protocols and devices.
The purpose of this patchset is for the userspace applications to continue doing
so with devicetree based systems without the need to fabricate fake hardware
description. And the hardware description will necessarily be fake in the case
when it is in fact userspace implementing and managing the drivers.

It's not to say that userspace drivers that duplicate kernel drivers cannot
examine the devicetree for configuration data. That's up to the userspace
driver writer to decide, however.

Thanks

Michal

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 12:35:40AM +0200, Michal Suchanek wrote:

>  config SPI_SPIDEV
> - tristate "User mode SPI device driver support"
> + bool "User mode SPI device driver support"

This is a step back, it would require spidev to be built in.

> - spin_lock_irq(>spi_lock);
> - spi = spi_dev_get(spidev->spi);
> - spin_unlock_irq(>spi_lock);
> + spi = filp->private_data;
> + spi = spi_dev_get(spi);

All this refactoring to move spidev about should've been a separate
patch, it's hard to find the actual content in here.

> - mutex_lock(_list_lock);
> + dev = bus_find_device(_bus_type, NULL, (void *) inode->i_rdev,
> +   spidev_devt_match);

...

> - dev_dbg(>spi->dev, "open/ENOMEM\n");
> + spi = to_spi_device(dev);
> +
> + mutex_lock(>buf_lock);
> + spin_lock_irq(>spidev_lock);
> + spi->spidev_users++;
> + spin_unlock_irq(>spidev_lock);

This is broken, it will unconditionally create a spidev for any chip
select regardless of if there's any actual hardware there and (even more
importantly) regardless if that hardware is actually a SPI device.  This
is not safe, especially given some of the ideas people seem to have for
userspaces.

I am getting completely fed up of saying this, it's not OK to just
expose pins to userspace when we have no idea what they are connected
to.


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 12:35:40AM +0200, Michal Suchanek wrote:

>  config SPI_SPIDEV
> - tristate "User mode SPI device driver support"
> + bool "User mode SPI device driver support"

This is a step back, it would require spidev to be built in.

> - spin_lock_irq(>spi_lock);
> - spi = spi_dev_get(spidev->spi);
> - spin_unlock_irq(>spi_lock);
> + spi = filp->private_data;
> + spi = spi_dev_get(spi);

All this refactoring to move spidev about should've been a separate
patch, it's hard to find the actual content in here.

> - mutex_lock(_list_lock);
> + dev = bus_find_device(_bus_type, NULL, (void *) inode->i_rdev,
> +   spidev_devt_match);

...

> - dev_dbg(>spi->dev, "open/ENOMEM\n");
> + spi = to_spi_device(dev);
> +
> + mutex_lock(>buf_lock);
> + spin_lock_irq(>spidev_lock);
> + spi->spidev_users++;
> + spin_unlock_irq(>spidev_lock);

This is broken, it will unconditionally create a spidev for any chip
select regardless of if there's any actual hardware there and (even more
importantly) regardless if that hardware is actually a SPI device.  This
is not safe, especially given some of the ideas people seem to have for
userspaces.

I am getting completely fed up of saying this, it's not OK to just
expose pins to userspace when we have no idea what they are connected
to.


signature.asc
Description: PGP signature