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

Groups > linux.kernel > #1379051 > unrolled thread

[PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-14 17:50 +0200
    Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 18:50 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Ira Weiny <ira.weiny@intel.com> - 2016-04-14 19:50 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 20:10 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-14 20:50 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 21:00 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-15 06:10 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Ira Weiny <ira.weiny@intel.com> - 2016-04-15 18:20 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Christoph Hellwig <hch@infradead.org> - 2016-04-15 19:40 +0200
              RE: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access "Hefty, Sean" <sean.hefty@intel.com> - 2016-04-15 19:50 +0200
              RE: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access "Woodruff, Robert J" <robert.j.woodruff@intel.com> - 2016-04-15 19:50 +0200
                Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-15 23:30 +0200
              Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-15 23:30 +0200
                Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Ira Weiny <ira.weiny@intel.com> - 2016-04-16 01:30 +0200
                  Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-16 08:20 +0200
                    Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-16 17:30 +0200
                Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-16 01:40 +0200
                  Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-16 08:10 +0200
                    Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 21:20 +0200
                      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-18 14:10 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-15 19:40 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-14 20:00 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 20:50 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-20 22:40 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-22 20:40 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-26 17:30 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Christoph Hellwig <hch@infradead.org> - 2016-04-18 15:10 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-18 19:50 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Christoph Hellwig <hch@infradead.org> - 2016-04-18 20:30 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Ira Weiny <ira.weiny@intel.com> - 2016-04-19 05:50 +0200
              Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Christoph Hellwig <hch@infradead.org> - 2016-04-19 20:50 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-19 19:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1380060 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 12:17:55PM -0400, Ira Weiny wrote:
> On Fri, Apr 15, 2016 at 07:01:26AM +0300, Leon Romanovsky wrote:
> > On Thu, Apr 14, 2016 at 01:48:31PM -0400, Ira Weiny wrote:
> > > On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> > > > On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> > > > > This patch series removes the write() interface for user access in favor of an
> > > > > ioctl() based approach. This is in response to the complaint that we had
> > > > > different handlers for write() and writev() doing different things and expecting
> > > > > different types of data. See:
> > > > 
> > > > I think we should wait on applying these patches until we globally sort out
> > > > what to do with the rdma uapi.
> > > > 
> > > > It just doesn't make alot of sense for drivers to have their own personal
> > > > char devices. :(
> > > 
> > > I'm afraid I have to disagree at this time.  Someday we may have "1 char device
> > > to rule them all" but right now we don't have any line of sight to that
> > > solution.  It may be _years_ before we can agree to the semantics which will
> > > work for all high speed, kernel bypass, rdma, low latency, network devices.
> > 
> > You didn't ever try to come and work on the solution. We talked about
> > finite time frame (_months_) which is doable based on knowledge that user
> > space parts are developed by the same companies and all our future changes
> > will be in one subsystem.
> 
> How can you say that I am not working on a solution?
> 
> We spent most of last week discussing possible solutions and I am in support of
> a more common core.

Great, did you show it to other RDMA stakeholders except Intel?
I saw nothing posted on ML or proposed for initial discussion, which
will be held in the next week or two.

It is a great opportunity to you guys to start and respect Linux kernel
collaboration development model and to stop to try to do it in your
corporate way.

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

#1379181

On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
>On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
>> This patch series removes the write() interface for user access in favor of an
>> ioctl() based approach. This is in response to the complaint that we had
>> different handlers for write() and writev() doing different things and expecting
>> different types of data. See:
>
>I think we should wait on applying these patches until we globally sort out
>what to do with the rdma uapi.

Perhaps there is a broader change to make to the rdma subsystem, but until 
that is fleshed out this patch set achieves our goal of fixing the 
write()/writev() problem and should be sufficient to let the driver come out 
of staging for 4.7?

>A second char dev for the eeprom? How is that OK? Why aren't you using
>the I2C layer for this?

I moved it because it is totally different in terms of functionality. The 
hfi1 device is for send/recv of packets across the wire. The eprom device is
for low level programming of the eprom on the chip. We do not use i2c for 
this because the eprom is directly attached to the chip and not accessible 
via i2c, requires register access.

>Why is there a snoop interface in here? How is that not something that
>belongs in a the core code?

The snoop interface is a low level diagnostic for the hfi. The intent is to 
grab packets before they are handed up to the verbs layer. It also lets us 
send all sorts of debug/diagnostic packets for testing.

-Denny

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

#1379218 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Thu, Apr 14, 2016 at 01:52:44PM -0400, Dennis Dalessandro wrote:
> On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> >On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> >>This patch series removes the write() interface for user access in favor of an
> >>ioctl() based approach. This is in response to the complaint that we had
> >>different handlers for write() and writev() doing different things and expecting
> >>different types of data. See:
> >
> >I think we should wait on applying these patches until we globally sort out
> >what to do with the rdma uapi.
> 
> Perhaps there is a broader change to make to the rdma subsystem, but until
> that is fleshed out this patch set achieves our goal of fixing the
> write()/writev() problem and should be sufficient to let the driver come out
> of staging for 4.7?

No. Al and Linus have clearly put the kibosh on the idea that a driver
gets a pass on whatever uAPI stuff they want just because it is in a
driver.

If anything adding uAPIs to drivers should be *harder* than adding
them to the core kernel. You nedd a lot more justification why the
core code shouldn't have a well designed version of the function.

You guys need to integrate with the rest of the kernel in some way,
this is just not OK. We catch so much flack from the rest of the
kernel community for our shitty uAPIs, we need to grow up.

I accept the argument that you need special high speed hardware
specific uAPIs for PSM - fine, but that doesn't give hfi1 a free pass
to add whatever other kooky things you find convenient. No to eeprom,
no to snoop.

If you want to migrate out of staging quickly then drop the uAPI from
the driver and submit a sane uAPI later as patches.

IMHO, it was a mistake for Roland to accept ipath with all this uAPI
stuff, and a double mistake to give qib an equal pass. hfi1 is adding
*even more* stuff, with flimsy justification. Enough is enough.

> >A second char dev for the eeprom? How is that OK? Why aren't you using
> >the I2C layer for this?
> 
> I moved it because it is totally different in terms of
> functionality. The

Nobody else is doing something like this. It is crazy.

Add a common RDMA API for eeprom. net has one under ethtool, it is
about time we grow something too, all the vendors seem to have various
hacks in this department. Maybe it fits under RDMA's growing netlink
footprint.

> >Why is there a snoop interface in here? How is that not something that
> >belongs in a the core code?
> 
> The snoop interface is a low level diagnostic for the hfi. The intent is to
> grab packets before they are handed up to the verbs layer. It also lets us
> send all sorts of debug/diagnostic packets for testing.

So? Why is that unique to hfi1? Packet capture is a well understood
multi-vendor thing.

Nobody else is getting a pass on uAPI design. Thing is, I don't think
it is actually hard to do a good job with the uAPI here, you just
actually have to try. :P

I told John I'd give you guys some design advice. I suggest you give
it a good think, make your wishlist and lets do something sane.

Jason

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

#1383730 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Thu, Apr 14, 2016 at 01:52:44PM -0400, Dennis Dalessandro wrote:
> The eprom device is for low level programming of the eprom on the
> chip. We do not use i2c for this because the eprom is directly
> attached to the chip and not accessible via i2c, requires register
> access.

Okay, but the twsi.c is still not acceptable in a driver, use the i2c
subsystem to talk to the qsfps. Open coding i2c is not OK.

The char dev(s) are also done wrong, there is no set to 'kobj.parent'
and there are empty release functions. There are certainly
use-after-free bugs between close and device removal, someone needs to
audit all of this carefully.

The patch forgets compat_ioctl.

Stuff like this should be a build bug:
 /* NOTE: assumes unsigned long is 8 bytes */

The 'goto on success' in hfi1_cdev_init is an anti-pattern, don't do it.

Even if the char dev stays, creating two whole sysfs classes seems
really unnecessary. Surely there is a better place to attach the cdev.

And this is just outrageous:

        if (atomic_inc_return(&user_count) == 1) {
                ret = hfi1_cdev_init(0, class_name(), &hfi1_file_ops,
                                     &wildcard_cdev, &wildcard_device,
                                     true);
                if (ret)

I can see an argument for a per-device char dev (as Christoph says,
that is not entirely uncommon) but this is a multi-device char dev????

It is not OK to create your own private subsystem in a driver! I count
*three* char devs before this series!?!?! One of them marked 0666 even!

I stopped looking at the code.

Jason

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

#1385432

On Wed, Apr 20, 2016 at 02:36:16PM -0600, Jason Gunthorpe wrote:
>On Thu, Apr 14, 2016 at 01:52:44PM -0400, Dennis Dalessandro wrote:
>> The eprom device is for low level programming of the eprom on the
>> chip. We do not use i2c for this because the eprom is directly
>> attached to the chip and not accessible via i2c, requires register
>> access.
>
>Okay, but the twsi.c is still not acceptable in a driver, use the i2c
>subsystem to talk to the qsfps. Open coding i2c is not OK.

Yeah, I mentioned this at OFA. We are looking at reworking this.

>The char dev(s) are also done wrong, there is no set to 'kobj.parent'
>and there are empty release functions. There are certainly
>use-after-free bugs between close and device removal, someone needs to
>audit all of this carefully.
>
>The patch forgets compat_ioctl.

We will take a look at this.

>Stuff like this should be a build bug:
> /* NOTE: assumes unsigned long is 8 bytes */

Our Kconfig depends on X86_64. Should we add a BUILD_BUG_ON or something?

>The 'goto on success' in hfi1_cdev_init is an anti-pattern, don't do it.

Can fix.

>Even if the char dev stays, creating two whole sysfs classes seems
>really unnecessary. Surely there is a better place to attach the cdev.
>
>And this is just outrageous:
>
>        if (atomic_inc_return(&user_count) == 1) {
>                ret = hfi1_cdev_init(0, class_name(), &hfi1_file_ops,
>                                     &wildcard_cdev, &wildcard_device,
>                                     true);
>                if (ret)
>
>I can see an argument for a per-device char dev (as Christoph says,
>that is not entirely uncommon) but this is a multi-device char dev????

We are discussing what can be done about this internally. On that note we 
are also looking at what we can do about the twsi as mentioned above, also 
the eprom, ui, and snoop. We'll send the actual plan for each issue to 
linux-rdma for feedback soon. 

We are certainly not opposed to improving these areas of the code. So long 
as we can maintain the same functionality, which seems doable. It's just 
that no one has spoken up about these things in the 9+ months since the 
driver was added. We need a bit of time to get it all sorted out.

-Denny

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

#1387617 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Fri, Apr 22, 2016 at 02:38:45PM -0400, Dennis Dalessandro wrote:
> >Stuff like this should be a build bug:
> >/* NOTE: assumes unsigned long is 8 bytes */
> 
> Our Kconfig depends on X86_64. Should we add a BUILD_BUG_ON or something?

Yes

> as we can maintain the same functionality, which seems doable. It's just
> that no one has spoken up about these things in the 9+ months since the
> driver was added. We need a bit of time to get it all sorted out.

Unfortunately this is a bit of a flaw with the staging process, really
it is not designed to handle uAPI work.

Jason

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

#1381681 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> > This patch series removes the write() interface for user access in favor of an
> > ioctl() based approach. This is in response to the complaint that we had
> > different handlers for write() and writev() doing different things and expecting
> > different types of data. See:
> 
> I think we should wait on applying these patches until we globally sort out
> what to do with the rdma uapi.
> 
> It just doesn't make alot of sense for drivers to have their own personal
> char devices. :(

I looked through the patches I tend to disagree - while we should wait
for a global UAPI for anything that's actually RDMA/verbs related these
seem to be misc little bits specific to the driver that have no business
in any sort of generic RDMA API.

> A second char dev for the eeprom? How is that OK? Why aren't you using
> the I2C layer for this?

... but this is a really good question, although the right layer to
plug this in would be the eeprom code in drivers/nvmem/

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

#1381950 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Mon, Apr 18, 2016 at 06:09:09AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> > On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> > > This patch series removes the write() interface for user access in favor of an
> > > ioctl() based approach. This is in response to the complaint that we had
> > > different handlers for write() and writev() doing different things and expecting
> > > different types of data. See:
> > 
> > I think we should wait on applying these patches until we globally sort out
> > what to do with the rdma uapi.
> > 
> > It just doesn't make alot of sense for drivers to have their own personal
> > char devices. :(
> 
> I looked through the patches I tend to disagree - while we should wait
> for a global UAPI for anything that's actually RDMA/verbs related these
> seem to be misc little bits specific to the driver that have no business
> in any sort of generic RDMA API.

I wasn't arguing this should integrate into verbs in some way, only
that the way to access the driver-specific uAPI of a RDMA device should
be through the RDMA common uAPI and not through a random char dev.

.. and of course that the driver-specific API be subject to a sane
review and use of the normal standards, not just written off as
driver-garbage nobody cares about. :(

For instance, if we had a driver specific channel, it casts this
endless stream of uAPI verbs patches in a different light: maybe they
should go down the driver-specific channel instead.

Jason

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

#1381966 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Mon, Apr 18, 2016 at 11:40:47AM -0600, Jason Gunthorpe wrote:
> I wasn't arguing this should integrate into verbs in some way, only
> that the way to access the driver-specific uAPI of a RDMA device should
> be through the RDMA common uAPI and not through a random char dev.

Well, it's stuff not related to our RDMA userspace API (which _is_
Verbs, not counting for the complete crackpot abuse in usnic), but
very device specific. 

The stuff the intel driver are doing isn't pretty, but unfortunately
not unusual either - lots of SCSI or network driver have ioctls
like that.  Now we could argue if the ioctls should be one the
main node (uverbs) or the a driver private chardev, or not exist
at all and people will have to patch the driver with some vendor
version if they really need it.  Examples for either of these
choices exist in the tree.

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

#1382148 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Mon, Apr 18, 2016 at 11:24:11AM -0700, Christoph Hellwig wrote:
> On Mon, Apr 18, 2016 at 11:40:47AM -0600, Jason Gunthorpe wrote:
> > I wasn't arguing this should integrate into verbs in some way, only
> > that the way to access the driver-specific uAPI of a RDMA device should
> > be through the RDMA common uAPI and not through a random char dev.
> 
> Well, it's stuff not related to our RDMA userspace API (which _is_
> Verbs, not counting for the complete crackpot abuse in usnic), but
> very device specific. 
> 
> The stuff the intel driver are doing isn't pretty, but unfortunately
> not unusual either - lots of SCSI or network driver have ioctls
> like that.  Now we could argue if the ioctls should be one the
> main node (uverbs) or the a driver private chardev, or not exist
> at all and people will have to patch the driver with some vendor
> version if they really need it.  Examples for either of these
> choices exist in the tree.

I'm a bit confused by what you are suggesting that "people will have to patch
the driver with some vendor version if they really need it."?

Could you elaborate?

PSM is the primary performant path for this device.  Without it this device is
severely limited in its intended functionality.

We are strongly motivated to have all of our functionality included in the
mainstream kernel.  So for eprom/snoop we would really like to find a way to
include all this functionality.

Ira

> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1382789 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Mon, Apr 18, 2016 at 11:45:49PM -0400, Ira Weiny wrote:
> I'm a bit confused by what you are suggesting that "people will have to patch
> the driver with some vendor version if they really need it."?
> 
> Could you elaborate?

There are lots of drivers where we simply did not accept these vendor
specific extensions at all.  Especially for networking drivers it's
pretty common.  I'm not proposing this here, just saying that we have
lots of examples for it.

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

#1382754 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Mon, Apr 18, 2016 at 11:24:11AM -0700, Christoph Hellwig wrote:
> On Mon, Apr 18, 2016 at 11:40:47AM -0600, Jason Gunthorpe wrote:
> > I wasn't arguing this should integrate into verbs in some way, only
> > that the way to access the driver-specific uAPI of a RDMA device should
> > be through the RDMA common uAPI and not through a random char dev.
> 
> Well, it's stuff not related to our RDMA userspace API (which _is_
> Verbs, not counting for the complete crackpot abuse in usnic), but
> very device specific. 

It is weakly related, it uses the same device discovery and security
model.

> The stuff the intel driver are doing isn't pretty, but unfortunately
> not unusual either - lots of SCSI or network driver have ioctls
> like that.  Now we could argue if the ioctls should be one the
> main node (uverbs) or the a driver private chardev, or not exist
> at all and people will have to patch the driver with some vendor
> version if they really need it.  Examples for either of these
> choices exist in the tree.

Right - and the RDMA uAPI has always had an integrated driver-bypass
channel as part of the verb uAPI calls, extending that to allow for
new-driver-specific calls seems very natural.

Jason

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web