Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570976 > unrolled thread

[PATCH 0/1] DRAFT: New Microsemi PCI Switch Management Driver

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/1] DRAFT: New Microsemi PCI Switch Management Driver Logan Gunthorpe <logang@deltatee.com> - 2017-01-31 19:30 +0100
    Re: [PATCH 1/1] MicroSemi Switchtec management interface driver Emil Velikov <emil.l.velikov@gmail.com> - 2017-01-31 22:00 +0100
      Re: [PATCH 1/1] MicroSemi Switchtec management interface driver Logan Gunthorpe <logang@deltatee.com> - 2017-02-01 00:20 +0100
        Re: [PATCH 1/1] MicroSemi Switchtec management interface driver Emil Velikov <emil.l.velikov@gmail.com> - 2017-02-01 13:20 +0100
          Re: [PATCH 1/1] MicroSemi Switchtec management interface driver Logan Gunthorpe <logang@deltatee.com> - 2017-02-02 17:40 +0100
            Re: [PATCH 1/1] MicroSemi Switchtec management interface driver Emil Velikov <emil.l.velikov@gmail.com> - 2017-02-03 15:00 +0100

#1570976 — [PATCH 0/1] DRAFT: New Microsemi PCI Switch Management Driver

Hi,

This is a continuation of the RFC we posted lasted month [1] which
proposes a management driver for Microsemi's Switchtec line of PCI
switches. This hardware is still looking to be used in the Open
Compute Platform

To make this entirely clear: the Switchtec products are compliant
with the PCI specifications and are supported today with the standard
in-kernel driver. However, these devices also expose a management endpoint
on a separate PCI function address which can be used to perform some
advanced operations. This is a driver for that function. See the patch
for more information.

Since the RFC, we've made the changes requested by Greg Kroah-Hartman
and Keith Busch, and we've also fleshed out a number of features. We've
added a couple of IOCTLs and sysfs attributes which are documented in
the patch. Significant work has also been done on the userspace tool
which is available under a GPL license at [2]. We've also had testing
done by some of the interested parties.

We hope to see this work included in either 4.11 or 4.12 assuming a
smooth review process.

The patch is based off of the v4.9 release.

Thanks for your review,

Logan

[1] https://www.spinics.net/lists/linux-pci/msg56897.html
[2] https://github.com/sbates130272/switchtec-user

--

Changes since RFC:

* Fixed incorrect use of the drive model as pointed out by Greg
  Kroah-Hartman
* Used devm functions as suggested by Keith Busch
* Added a handful of sysfs attributes to the switchtec class
* Added a handful of IOCTLs to the switchtec device
* A number of miscelaneous bug fixes

--

Logan Gunthorpe (1):
  MicroSemi Switchtec management interface driver

 Documentation/ABI/testing/sysfs-class-switchtec |   96 ++
 Documentation/ioctl/ioctl-number.txt            |    1 +
 Documentation/switchtec.txt                     |   80 ++
 MAINTAINERS                                     |   11 +
 drivers/pci/Kconfig                             |    1 +
 drivers/pci/Makefile                            |    1 +
 drivers/pci/switch/Kconfig                      |   13 +
 drivers/pci/switch/Makefile                     |    1 +
 drivers/pci/switch/switchtec.c                  | 1320 +++++++++++++++++++++++
 drivers/pci/switch/switchtec.h                  |  266 +++++
 include/uapi/linux/switchtec_ioctl.h            |  129 +++
 11 files changed, 1919 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-switchtec
 create mode 100644 Documentation/switchtec.txt
 create mode 100644 drivers/pci/switch/Kconfig
 create mode 100644 drivers/pci/switch/Makefile
 create mode 100644 drivers/pci/switch/switchtec.c
 create mode 100644 drivers/pci/switch/switchtec.h
 create mode 100644 include/uapi/linux/switchtec_ioctl.h

--
2.1.4

[toc] | [next] | [standalone]

#1571079 — Re: [PATCH 1/1] MicroSemi Switchtec management interface driver

Hi Logan,

NOTE: Please take my comments with a healthy pinch of salt.

I'd imagine that core/more experienced developers have more thorough
feedback, so I'll mention a few things on the less common part -
robust/compat UABI.
Above all, please read through the in-tree documentation on the topic [1]

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt?id=refs/tags/v4.10-rc6

On 31 January 2017 at 17:03, Logan Gunthorpe <logang@deltatee.com> wrote:

> --- /dev/null
> +++ b/include/uapi/linux/switchtec_ioctl.h


> +enum switchtec_ioctl_partition {

> +       SWITCHTEC_IOCTL_NUM_PARTITIONS,
> +};
I'm not sure what the overall consensus on the topic of 'enums vs
defines in UABI'. Fwiw personally I'm in favour of defines since enums
can lead to subtle issues if one is not extra careful. For example:
 - add new enum amidst the list - ABI break
 - add new enum at the end of the list [just before NUM_PARTITIONS] - ABI break.
 - opt for negative value for existing/new enum - ABI break
 - other

With a variable size in mind, carefully consider how you'll copy data
back and forth in the case of new kernel (supports partition[X) + old
userspace (partition [X-1]) and vice-versa. Feel free to look at the
drm ioctl handler and/or others through the kernel.

Hint - as-is we'll end up with buffer overflows/other issues.

> +
> +struct switchtec_ioctl_fw_info {
> +       __u32 flash_length;
> +
> +       struct {
> +               __u32 address;
> +               __u32 length;
> +               __u32 active;
Something to keep in mind, not sure how likely it is here:
If you embed structs (partition in this case), you will not be able to
extend it [the embedded one] it in the future without the risk of ABI
breakage.

> +       } partition[SWITCHTEC_IOCTL_NUM_PARTITIONS];
> +};
> +
Afaict this and most/all other structs [in this patch] are in
violation of Prerequisites#2 and/or #3.
Personally I find pahole quite useful - reference all the UABI structs
in a dummy userspace app, compile with gcc -g -O0 and observe the
output across 32 and 64bit builds. Members _must_ be at the same
offset, otherwise things will be broken with 64bit kernel on a 32bit
userspace. Something which people will eventually end up trying/using,
even if you don't plan to support it.

Please check that Basics (#2 and #5 in particular) are also covered.

A couple more generic suggestions, which I may have noticed while
skimming through:
 - please ensure that all the input is properly sanitised, before,
going into the actual implementation
Afaict having the separate step/separation helps clarity and reduces
chances of you/others getting it wrong down the line.
 - user provided storage must not be changed when the ioctl fails.

Additionally you want to provide a reference to open-source userspace
[alongside acknowledgement of the maintainers/co-developers about the
design] which makes use of said IOCTL(s). But please, do _not_ merge
userspace code before the kernel work has landed.

Hope that provided you with sufficient good points wrt IOCTL design.

Regards,
Emil

[toc] | [prev] | [next] | [standalone]

#1571204 — Re: [PATCH 1/1] MicroSemi Switchtec management interface driver

Hi Emil,

Thanks for the feedback.

On 31/01/17 01:48 PM, Emil Velikov wrote:
>> +struct switchtec_ioctl_fw_info {
>> +       __u32 flash_length;
>> +
>> +       struct {
>> +               __u32 address;
>> +               __u32 length;
>> +               __u32 active;
> Something to keep in mind, not sure how likely it is here:
> If you embed structs (partition in this case), you will not be able to
> extend it [the embedded one] it in the future without the risk of ABI
> breakage.

Based on this feedback, I'll rework the fw_info IOCTL so the user only
requests one partition at a time. This will get rid of the embedded
struct and any concerns about size changes. I was originally trying to
make the ioctl RO to make it simpler but that maybe isn't the case.

>> +       } partition[SWITCHTEC_IOCTL_NUM_PARTITIONS];
>> +};
>> +
> Afaict this and most/all other structs [in this patch] are in
> violation of Prerequisites#2 and/or #3.
> Personally I find pahole quite useful - reference all the UABI structs
> in a dummy userspace app, compile with gcc -g -O0 and observe the
> output across 32 and 64bit builds. Members _must_ be at the same
> offset, otherwise things will be broken with 64bit kernel on a 32bit
> userspace. Something which people will eventually end up trying/using,
> even if you don't plan to support it.

I've made some changes to the padding and sizes to accommodate this.
I'll also do some testing with pahole before v2.


> Please check that Basics (#2 and #5 in particular) are also covered.

> then you need a driver feature flag or revision number somewhere.

We don't have this specifically. A new version ioctl could be added
later when and if changes occur; or the userspace code could easily read
the module version through sysfs and we could increment that with ioctl
changes.

> A couple more generic suggestions, which I may have noticed while
> skimming through:
>  - please ensure that all the input is properly sanitised, before,
> going into the actual implementation
> Afaict having the separate step/separation helps clarity and reduces
> chances of you/others getting it wrong down the line.

The inputs are indeed properly checked in the code. Most of the ioctls
that take inputs are so simple it's hard to separate the two steps. ie.
Verifying that the input is right pretty much gives you the answer you
need to send back to userspace.

>  - user provided storage must not be changed when the ioctl fails.

This should already be true.

> Additionally you want to provide a reference to open-source userspace
> [alongside acknowledgement of the maintainers/co-developers about the
> design] which makes use of said IOCTL(s). But please, do _not_ merge
> userspace code before the kernel work has landed.

I'm having trouble understanding what you're asking here. We provided a
reference to the userspace code in the commit message. Currently the
userspace code is completely useless without the kernel module and it is
entirely independent of other projects. I'm also the only committer so
far on the userspace code. So I assume this only applies if we were
merging changes to other existing code?

> Hope that provided you with sufficient good points wrt IOCTL design.

Thanks, this was very helpful.

Logan

[toc] | [prev] | [next] | [standalone]

#1571502 — Re: [PATCH 1/1] MicroSemi Switchtec management interface driver

On 31 January 2017 at 23:13, Logan Gunthorpe <logang@deltatee.com> wrote:
> Hi Emil,
>
> Thanks for the feedback.
>
> On 31/01/17 01:48 PM, Emil Velikov wrote:
>>> +struct switchtec_ioctl_fw_info {
>>> +       __u32 flash_length;
>>> +
>>> +       struct {
>>> +               __u32 address;
>>> +               __u32 length;
>>> +               __u32 active;
>> Something to keep in mind, not sure how likely it is here:
>> If you embed structs (partition in this case), you will not be able to
>> extend it [the embedded one] it in the future without the risk of ABI
>> breakage.
>
> Based on this feedback, I'll rework the fw_info IOCTL so the user only
> requests one partition at a time. This will get rid of the embedded
> struct and any concerns about size changes. I was originally trying to
> make the ioctl RO to make it simpler but that maybe isn't the case.
>
You can keep it roughly as-is if you're ~reasonably certain one won't
change it in the future.

>> then you need a driver feature flag or revision number somewhere.
>
> We don't have this specifically. A new version ioctl could be added
> later when and if changes occur; or the userspace code could easily read
> the module version through sysfs and we could increment that with ioctl
> changes.
>
Some teams frown upon adding new IOCTL(s) where existing ones can be
made backward/forward compatible.
I'm not fully aware of the general direction/consensus on the topic,
so it might be a minority.

On the other hand, reading through sysfs for module version in order
to use IOCTL A or B sounds quite hacky. Do you have an example where
this is used or pointed out as good approach ?

>> A couple more generic suggestions, which I may have noticed while
>> skimming through:
>>  - please ensure that all the input is properly sanitised, before,
>> going into the actual implementation
>> Afaict having the separate step/separation helps clarity and reduces
>> chances of you/others getting it wrong down the line.
>
> The inputs are indeed properly checked in the code. Most of the ioctls
> that take inputs are so simple it's hard to separate the two steps. ie.
> Verifying that the input is right pretty much gives you the answer you
> need to send back to userspace.
>
>>  - user provided storage must not be changed when the ioctl fails.
>
> This should already be true.
>
You're spot on [for both points]. I must have imagined something.

>> Additionally you want to provide a reference to open-source userspace
>> [alongside acknowledgement of the maintainers/co-developers about the
>> design] which makes use of said IOCTL(s). But please, do _not_ merge
>> userspace code before the kernel work has landed.
>
> I'm having trouble understanding what you're asking here. We provided a
> reference to the userspace code in the commit message. Currently the
> userspace code is completely useless without the kernel module and it is
> entirely independent of other projects. I'm also the only committer so
> far on the userspace code. So I assume this only applies if we were
> merging changes to other existing code?
>
Yes, I'm blind - you have links to the userspace.

Afaict the idea is to not ship/bundle/release userspace until kernel
parts are in.
The "do not commit the changes" is implied as [very rarely] distros
package from "random" git checkouts. Leading to all sorts of fun when
it is mismatched wrt the kernel parts. Likelihood of doing that here
is virtually none here, so this is a JFYI inspired by some past
experiences.

>> Hope that provided you with sufficient good points wrt IOCTL design.
>
> Thanks, this was very helpful.
>
Glad to hear. Then again you already had most of the things nicely done, imho.

-Emil

[toc] | [prev] | [next] | [standalone]

#1572507 — Re: [PATCH 1/1] MicroSemi Switchtec management interface driver

On 01/02/17 05:10 AM, Emil Velikov wrote:
> You can keep it roughly as-is if you're ~reasonably certain one won't
> change it in the future.

I've made the change anyway. I think it's better now.

> Some teams frown upon adding new IOCTL(s) where existing ones can be
> made backward/forward compatible.
> I'm not fully aware of the general direction/consensus on the topic,
> so it might be a minority.

Sure, I just don't know what might be needed in the future so it's hard
to add a version or flags ioctl now.

> On the other hand, reading through sysfs for module version in order
> to use IOCTL A or B sounds quite hacky. Do you have an example where
> this is used or pointed out as good approach ?

I don't know of anything doing it that way now. But it sure would be
easy and make a bit of sense. (We'd actually use the module version for
something useful.) Either way, it would really depend on if and how
things change in the future. The point is there are options to expand if
needed.


> Afaict the idea is to not ship/bundle/release userspace until kernel
> parts are in.
> The "do not commit the changes" is implied as [very rarely] distros
> package from "random" git checkouts. Leading to all sorts of fun when
> it is mismatched wrt the kernel parts. Likelihood of doing that here
> is virtually none here, so this is a JFYI inspired by some past
> experiences.

Understood.

> Glad to hear. Then again you already had most of the things nicely done, imho.

Great, thanks.

Logan

[toc] | [prev] | [next] | [standalone]

#1573103 — Re: [PATCH 1/1] MicroSemi Switchtec management interface driver

On 2 February 2017 at 16:37, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 01/02/17 05:10 AM, Emil Velikov wrote:
>> You can keep it roughly as-is if you're ~reasonably certain one won't
>> change it in the future.
>
> I've made the change anyway. I think it's better now.
>
>> Some teams frown upon adding new IOCTL(s) where existing ones can be
>> made backward/forward compatible.
>> I'm not fully aware of the general direction/consensus on the topic,
>> so it might be a minority.
>
> Sure, I just don't know what might be needed in the future so it's hard
> to add a version or flags ioctl now.
>
Yes knowing how things will need to change in the is hard. That's why
the documentation suggestions adding a flag to the ioctl structs.
It [the flag] might imply certain functional/implementation change,
support for new/deprecation of old features and others.

>> On the other hand, reading through sysfs for module version in order
>> to use IOCTL A or B sounds quite hacky. Do you have an example where
>> this is used or pointed out as good approach ?
>
> I don't know of anything doing it that way now. But it sure would be
> easy and make a bit of sense. (We'd actually use the module version for
> something useful.) Either way, it would really depend on if and how
> things change in the future. The point is there are options to expand if
> needed.
>
The part that nobody else is doing such a thing should ring a bell ;-)

It's no my call, but if it was I'd stick with the existing approach
and not "reinvent the wheel" sort of speak.

Thanks
Emil

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web