[RFC] Proposal to address hfi1 UI and EPROM devices

[RFC] Proposal to address hfi1 UI and EPROM devices

[Dennis Dalessandro]

The hfi1 driver has a "UI" device associated with it. This is currently a
char dev. It is used for debugging/diagnostics. Basically it provides direct
user level access to HW registers. We have been exploring options to remove
this char dev, yet retain the same functionality.

One approach we have considered is using the regmap interface. This has a
number of problems. It is primarily a read only interface. The write
capability is not even available as a compile time option. A code change is
needed to make use of it. There are other issues as well such as being 32bit
when we need 64bit. So this seems like the wrong option.

Another approach is using the driver's resource0 file. We can mmap() the BAR
and access the registers. The main drawback we have with that is we have
some hardware requirements that the driver needs to arbitrate access to
certain registers. This means some form of kernel/user space shared locking.  
We are currently not aware of anything that the kernel provides to achieve 
this. However, we are still looking and would appreciate any pointers.

The third approach would be to take our existing UI implementation and plop
it down in debugfs as a binary file. We can control our locking just as we
do today, in the driver. This gets rid of the character device, and it's not
even available unless the admin decides to mount debugfs.

We also should be able to use any of these schemes to handle our eprom
reading/writing. Adding eprom to IPoIB as Doug suggested is a fine plan, but
we technically don't need to do it right now when we could make do with the
"UI" functionality and hide the details in user space. In the future there 
may well be value in having an eprom capability in the rdma sub-system, but 
for now we believe we can avoid extending the kernel in this regard.

-Denny

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Mon, May 02, 2016 at 03:55:02PM -0400, Dennis Dalessandro wrote:
> We also should be able to use any of these schemes to handle our eprom
> reading/writing. Adding eprom to IPoIB as Doug suggested is a fine plan, but
> we technically don't need to do it right now when we could make do with the
> "UI" functionality and hide the details in user space. In the future there
> may well be value in having an eprom capability in the rdma sub-system, but
> for now we believe we can avoid extending the kernel in this regard.

Just to understand better your RFC. Are you asking from the community
excuse do not implement core functionality just because you don't need
it? Am I right?

Thanks

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Dennis Dalessandro]

On Tue, May 03, 2016 at 07:24:57PM +0300, Leon Romanovsky wrote:
>On Mon, May 02, 2016 at 03:55:02PM -0400, Dennis Dalessandro wrote:
>> We also should be able to use any of these schemes to handle our eprom
>> reading/writing. Adding eprom to IPoIB as Doug suggested is a fine plan, but
>> we technically don't need to do it right now when we could make do with the
>> "UI" functionality and hide the details in user space. In the future there
>> may well be value in having an eprom capability in the rdma sub-system, but
>> for now we believe we can avoid extending the kernel in this regard.
>
>Just to understand better your RFC. Are you asking from the community
>excuse do not implement core functionality just because you don't need
>it? Am I right?

The purpose of the RFC is to get the community's feedback on our plans to 
solve our UI/EPROM issue. The paragraphs [1] not snipped in your response 
focus on that. It comes down to:

Use resource0 which does not support locking vs use a file in 
/sys/kernel/debug.

The point of the particular paragraph you are questioning is that I don't 
think we should add more code and complexity to the kernel for something 
that is not needed.

[1] http://marc.info/?l=linux-rdma&m=146221891012428&w=2

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Jason Gunthorpe]

On Mon, May 02, 2016 at 03:55:02PM -0400, Dennis Dalessandro wrote:
> One approach we have considered is using the regmap interface. This has a
> number of problems. It is primarily a read only interface. The write
> capability is not even available as a compile time option. A code change is
> needed to make use of it. There are other issues as well such as being 32bit
> when we need 64bit. So this seems like the wrong option.

Yah, the places I've seen regmap used don't seem to match this use
case.

> Another approach is using the driver's resource0 file. We can mmap() the BAR
> and access the registers. The main drawback we have with that is we have
> some hardware requirements that the driver needs to arbitrate access to
> certain registers. This means some form of kernel/user space shared locking.
> We are currently not aware of anything that the kernel provides to achieve
> this. However, we are still looking and would appreciate any pointers.

You should probably use resource0 as much as possible - that minimizes
the kernel code required.

> The third approach would be to take our existing UI implementation and plop
> it down in debugfs as a binary file. We can control our locking just as we
> do today, in the driver. This gets rid of the character device, and it's not
> even available unless the admin decides to mount debugfs.

.. and use debugfs just for that little bit of shared locking you need
to make resource0 work out.

Minimizing the kernel impact for debugging code is generally desirable.

> We also should be able to use any of these schemes to handle our eprom
> reading/writing. Adding eprom to IPoIB as Doug suggested is a fine
> plan, but

I would be fine with this if you moved the eeprom handling to user
space and used resource0.

Less fine if the code remains in the kernel but just moves to debugfs.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

On Tue, May 03, 2016 at 12:54:05PM -0400, Dennis Dalessandro wrote:
> On Tue, May 03, 2016 at 07:24:57PM +0300, Leon Romanovsky wrote:
> >On Mon, May 02, 2016 at 03:55:02PM -0400, Dennis Dalessandro wrote:
> >>We also should be able to use any of these schemes to handle our eprom
> >>reading/writing. Adding eprom to IPoIB as Doug suggested is a fine plan, but
> >>we technically don't need to do it right now when we could make do with the
> >>"UI" functionality and hide the details in user space. In the future there
> >>may well be value in having an eprom capability in the rdma sub-system, but
> >>for now we believe we can avoid extending the kernel in this regard.
> >
> >Just to understand better your RFC. Are you asking from the community
> >excuse do not implement core functionality just because you don't need
> >it? Am I right?
> 
> The purpose of the RFC is to get the community's feedback on our plans to
> solve our UI/EPROM issue. The paragraphs [1] not snipped in your response
> focus on that. It comes down to:
> 
> Use resource0 which does not support locking vs use a file in
> /sys/kernel/debug.

I didn't quote the whole email because I didn't see any question in
these 3 options:
* First option - doesn't meet the requirements and hard to extend.
* Second option - doesn't meet the requirements and HW limitations
* Third option - presented as one possible option and looks like the
  correct one.

And the real question is "where" do you need to implement third option.

> 
> The point of the particular paragraph you are questioning is that I don't
> think we should add more code and complexity to the kernel for something
> that is not needed.
> 
> [1] http://marc.info/?l=linux-rdma&m=146221891012428&w=2
> 
> -Denny

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2239 bytes --]

On Tue, May 03, 2016 at 09:42:18PM +0300, Leon Romanovsky wrote:
> On Tue, May 03, 2016 at 12:54:05PM -0400, Dennis Dalessandro wrote:
> > On Tue, May 03, 2016 at 07:24:57PM +0300, Leon Romanovsky wrote:
> > >On Mon, May 02, 2016 at 03:55:02PM -0400, Dennis Dalessandro wrote:
> > >>We also should be able to use any of these schemes to handle our eprom
> > >>reading/writing. Adding eprom to IPoIB as Doug suggested is a fine plan, but
> > >>we technically don't need to do it right now when we could make do with the
> > >>"UI" functionality and hide the details in user space. In the future there
> > >>may well be value in having an eprom capability in the rdma sub-system, but
> > >>for now we believe we can avoid extending the kernel in this regard.
> > >
> > >Just to understand better your RFC. Are you asking from the community
> > >excuse do not implement core functionality just because you don't need
> > >it? Am I right?
> > 
> > The purpose of the RFC is to get the community's feedback on our plans to
> > solve our UI/EPROM issue. The paragraphs [1] not snipped in your response
> > focus on that. It comes down to:
> > 
> > Use resource0 which does not support locking vs use a file in
> > /sys/kernel/debug.
> 
> I didn't quote the whole email because I didn't see any question in
> these 3 options:
> * First option - doesn't meet the requirements and hard to extend.
> * Second option - doesn't meet the requirements and HW limitations
> * Third option - presented as one possible option and looks like the
>   correct one.
> 
> And the real question is "where" do you need to implement third option.

And to remove the tension, all what you was supposed to ask can be
summarized in one question: "Do we have other customers for EEPROM in
our RDMA stack?". If the answer is yes, you will need to implement it in
core, and if the answer is no, you will implement it in your driver.

Hope it helps.

> 
> > 
> > The point of the particular paragraph you are questioning is that I don't
> > think we should add more code and complexity to the kernel for something
> > that is not needed.
> > 
> > [1] http://marc.info/?l=linux-rdma&m=146221891012428&w=2
> > 
> > -Denny



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Dennis Dalessandro]

On Tue, May 03, 2016 at 11:31:30AM -0600, Jason Gunthorpe wrote:

>> Another approach is using the driver's resource0 file. We can mmap() the BAR
>> and access the registers. The main drawback we have with that is we have
>> some hardware requirements that the driver needs to arbitrate access to
>> certain registers. This means some form of kernel/user space shared locking.
>> We are currently not aware of anything that the kernel provides to achieve
>> this. However, we are still looking and would appreciate any pointers.
>
>You should probably use resource0 as much as possible - that minimizes
>the kernel code required.
>
>> The third approach would be to take our existing UI implementation and plop
>> it down in debugfs as a binary file. We can control our locking just as we
>> do today, in the driver. This gets rid of the character device, and it's not
>> even available unless the admin decides to mount debugfs.
>
>.. and use debugfs just for that little bit of shared locking you need
>to make resource0 work out.

Yeah. I was hoping we could find some other kernel facility that can provide 
a locking type mechanism between the driver and user space rather than go 
roll our own. We'll come up with something more concrete in the form of 
patch and post soon though.

>Minimizing the kernel impact for debugging code is generally desirable.

Agree. Especially in this case where we should be able to make do with the 
already provided facilities. 

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Dennis Dalessandro]

On Tue, May 03, 2016 at 09:42:18PM +0300, Leon Romanovsky wrote:
>On Tue, May 03, 2016 at 12:54:05PM -0400, Dennis Dalessandro wrote:
>> On Tue, May 03, 2016 at 07:24:57PM +0300, Leon Romanovsky wrote:
>> >On Mon, May 02, 2016 at 03:55:02PM -0400, Dennis Dalessandro wrote:
>> >>We also should be able to use any of these schemes to handle our eprom
>> >>reading/writing. Adding eprom to IPoIB as Doug suggested is a fine plan, but
>> >>we technically don't need to do it right now when we could make do with the
>> >>"UI" functionality and hide the details in user space. In the future there
>> >>may well be value in having an eprom capability in the rdma sub-system, but
>> >>for now we believe we can avoid extending the kernel in this regard.
>> >
>> >Just to understand better your RFC. Are you asking from the community
>> >excuse do not implement core functionality just because you don't need
>> >it? Am I right?
>> 
>> The purpose of the RFC is to get the community's feedback on our plans to
>> solve our UI/EPROM issue. The paragraphs [1] not snipped in your response
>> focus on that. It comes down to:
>> 
>> Use resource0 which does not support locking vs use a file in
>> /sys/kernel/debug.
>
>I didn't quote the whole email because I didn't see any question in
>these 3 options:
>* First option - doesn't meet the requirements and hard to extend.
>* Second option - doesn't meet the requirements and HW limitations
>* Third option - presented as one possible option and looks like the
>  correct one.
>
>And the real question is "where" do you need to implement third option.

As in where would it be visible? That would be in /sys/kernel/debug/hfi1.  
We have some counters and stats there already.  Based on Jason's response 
I'm thinking we can use that just for the locking aspect.

-Denny

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Dennis Dalessandro]

On Wed, May 04, 2016 at 07:41:07AM +0300, Leon Romanovsky wrote:
>> I didn't quote the whole email because I didn't see any question in
>> these 3 options:
>> * First option - doesn't meet the requirements and hard to extend.
>> * Second option - doesn't meet the requirements and HW limitations
>> * Third option - presented as one possible option and looks like the
>>   correct one.
>> 
>> And the real question is "where" do you need to implement third option.
>
>And to remove the tension, all what you was supposed to ask can be
>summarized in one question: "Do we have other customers for EEPROM in
>our RDMA stack?". If the answer is yes, you will need to implement it in
>core, and if the answer is no, you will implement it in your driver.
>
>Hope it helps.

I think it's slightly more complicated than that. There are three options 
really: core, driver, or get it out of the kernel.

Whether we have other things that need EEPROM in the RDMA stack is only part 
of the question. We also should consider: do we have customers for EEPROM in 
the RDMA stack that could program/access said EEPROM using user space and 
are instead doing it in a driver?

If that is the case then I'd say they should follow the path we are planning 
for hfi1 and remove the code from the kernel. If there is already a way to 
do something from user space that should be leveraged as much as possible.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Jason Gunthorpe]

On Wed, May 04, 2016 at 08:36:21AM -0400, Dennis Dalessandro wrote:

> I think it's slightly more complicated than that. There are three options
> really: core, driver, or get it out of the kernel.

>From a core maintenance perspective, I think it is very simple, if
someone wishes to add code to their driver to manipulate the EEPROM,
then at this point the work to make a common uAPI falls on to their
shoulders.

I'd also make a very clear message to driver submitters: Do not
include uAPIs in your initial driver patch set. Those should follow on
as dedicated well identified patches so that they attracted the proper
review.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1180 bytes --]

On Wed, May 04, 2016 at 12:15:09PM -0600, Jason Gunthorpe wrote:
> On Wed, May 04, 2016 at 08:36:21AM -0400, Dennis Dalessandro wrote:
> > I think it's slightly more complicated than that. There are three options
> > really: core, driver, or get it out of the kernel.
> 
> From a core maintenance perspective, I think it is very simple, if
> someone wishes to add code to their driver to manipulate the EEPROM,
> then at this point the work to make a common uAPI falls on to their
> shoulders.

Agree,
And before rushing to code this functionality, please double check that
there is no already implemented such similar common functionality in other
subsystems.

I bet that you already did it.

> 
> I'd also make a very clear message to driver submitters: Do not
> include uAPIs in your initial driver patch set. Those should follow on
> as dedicated well identified patches so that they attracted the proper
> review.

It is hard to agree with this point.
It contradicts to development model of submitting whole feature at once
and not unconnected piece of code, which someone will be needed to
maintain without any real user behind it.

> 
> Jason

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Dennis Dalessandro]

On Thu, May 05, 2016 at 09:38:34AM +0300, Leon Romanovsky wrote:
>> I'd also make a very clear message to driver submitters: Do not
>> include uAPIs in your initial driver patch set. Those should follow on
>> as dedicated well identified patches so that they attracted the proper
>> review.
>
>It is hard to agree with this point.
>It contradicts to development model of submitting whole feature at once
>and not unconnected piece of code, which someone will be needed to
>maintain without any real user behind it.

I think the message really should be that if your driver contains uAPI 
changes those should be in separate patches that are clearly identified. So 
if you have a driver that is developed off-list initially, instead of just 
breaking it up into chunks for submission add another step.

Something like this:
1) Submit patch series which break-ups internally developed code
2) Submit patch series with separated out uAPI code
3) Submit patch that makes the build go-live

These can all be submitted together, but with the patches broken up like 
this reviewers can target uAPI code more easily.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Thu, May 05, 2016 at 08:00:34AM -0400, Dennis Dalessandro wrote:
> On Thu, May 05, 2016 at 09:38:34AM +0300, Leon Romanovsky wrote:
> >>I'd also make a very clear message to driver submitters: Do not
> >>include uAPIs in your initial driver patch set. Those should follow on
> >>as dedicated well identified patches so that they attracted the proper
> >>review.
> >
> >It is hard to agree with this point.
> >It contradicts to development model of submitting whole feature at once
> >and not unconnected piece of code, which someone will be needed to
> >maintain without any real user behind it.
> 
> I think the message really should be that if your driver contains uAPI
> changes those should be in separate patches that are clearly identified. So
> if you have a driver that is developed off-list initially, instead of just
> breaking it up into chunks for submission add another step.
> 
> Something like this:
> 1) Submit patch series which break-ups internally developed code
> 2) Submit patch series with separated out uAPI code
> 3) Submit patch that makes the build go-live
> 
> These can all be submitted together, but with the patches broken up like
> this reviewers can target uAPI code more easily.

At the end, there is no point of accepting (1) without finished review
of (2 and 3). Right now all patch series already have such internal separation
in a slightly different order.

I'm as a reviewer prefer to see whole picture and follow logic from the
general entry point to the specific implementation in driver.

> 
> -Denny

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Jason Gunthorpe]

On Thu, May 05, 2016 at 03:39:32PM +0300, Leon Romanovsky wrote:

> > I think the message really should be that if your driver contains uAPI
> > changes those should be in separate patches that are clearly identified. So
> > if you have a driver that is developed off-list initially, instead of just
> > breaking it up into chunks for submission add another step.
> > 
> > Something like this:
> > 1) Submit patch series which break-ups internally developed code
> > 2) Submit patch series with separated out uAPI code
> > 3) Submit patch that makes the build go-live

Yes. Perhaps even submit #2 after getting #1 mainlined.

Make it easy to find the important/controversial things and the review
process will work much better for everyone. Buring stuff in a monster
patch is just going to stretch it out.

> > These can all be submitted together, but with the patches broken up like
> > this reviewers can target uAPI code more easily.
 
> At the end, there is no point of accepting (1) without finished review
> of (2 and 3). Right now all patch series already have such internal separation
> in a slightly different order.

Eh?

Drivers should be able to stand alone without dedicated uapis
(excluding the udata stuff).

For instance, the HFI1 driver is as functional as any other RDMA
driver without it's cdev, eeprom, debug and sysfs uAPIs. Those are all
value add features that do not impact the driver's ability to operate
as an RDMA device.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On 05/05/2016 02:08 PM, Jason Gunthorpe wrote:
> On Thu, May 05, 2016 at 03:39:32PM +0300, Leon Romanovsky wrote:
> 
>>> I think the message really should be that if your driver contains uAPI
>>> changes those should be in separate patches that are clearly identified. So
>>> if you have a driver that is developed off-list initially, instead of just
>>> breaking it up into chunks for submission add another step.
>>>
>>> Something like this:
>>> 1) Submit patch series which break-ups internally developed code
>>> 2) Submit patch series with separated out uAPI code
>>> 3) Submit patch that makes the build go-live
> 
> Yes. Perhaps even submit #2 after getting #1 mainlined.
> 
> Make it easy to find the important/controversial things and the review
> process will work much better for everyone. Buring stuff in a monster
> patch is just going to stretch it out.
> 
>>> These can all be submitted together, but with the patches broken up like
>>> this reviewers can target uAPI code more easily.
>  
>> At the end, there is no point of accepting (1) without finished review
>> of (2 and 3). Right now all patch series already have such internal separation
>> in a slightly different order.
> 
> Eh?
> 
> Drivers should be able to stand alone without dedicated uapis
> (excluding the udata stuff).

That's probably true most of the time...

> For instance, the HFI1 driver is as functional as any other RDMA
> driver without it's cdev, eeprom, debug and sysfs uAPIs. Those are all
> value add features that do not impact the driver's ability to operate
> as an RDMA device.

In this case though, not so much.  The qib and hfi1 devices have always
been PSM devices first, verbs devices merely as a compat layer.  It's
easier to provides a verbs interface for kernel ULPs than it is to write
PSM in the kernel.  But their hardware is designed around the way PSM
uses it, and how verbs makes use of it is decidedly sub-optimal.  They
could do without the eeprom, debug, and sysfs stuff, but the cdev and
PSM not so much.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Jason Gunthorpe]

On Thu, May 05, 2016 at 02:36:16PM -0400, Doug Ledford wrote:
> > For instance, the HFI1 driver is as functional as any other RDMA
> > driver without it's cdev, eeprom, debug and sysfs uAPIs. Those are all
> > value add features that do not impact the driver's ability to operate
> > as an RDMA device.
> 
> In this case though, not so much.  The qib and hfi1 devices have always
> been PSM devices first, verbs devices merely as a compat layer.
> It's

The driver can do both interfaces, and the hardware was clearly
designed to accelerate the typical verbs operations as well as support
the psm style of operation.

> easier to provides a verbs interface for kernel ULPs than it is to write
> PSM in the kernel.  But their hardware is designed around the way PSM
> uses it, and how verbs makes use of it is decidedly sub-optimal.  They
> could do without the eeprom, debug, and sysfs stuff, but the cdev and
> PSM not so much.

I didn't say the cdev shouldn't exist, only that the core driver should
have been submitted first and then patched to add in the other stuff,
maybe in the same series, but certainly split up.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2702 bytes --]

On 05/02/2016 03:55 PM, Dennis Dalessandro wrote:
[ snip stuff about the UI file ]

The UI file discussion appears to have reached a reasonable conclusion,
so I'm not going to add to it.

> We also should be able to use any of these schemes to handle our eprom
> reading/writing. Adding eprom to IPoIB as Doug suggested is a fine plan,
> but
> we technically don't need to do it right now when we could make do with the
> "UI" functionality and hide the details in user space. In the future
> there may well be value in having an eprom capability in the rdma
> sub-system, but for now we believe we can avoid extending the kernel in
> this regard.

I'm not sure I agree here.

First, on the "technically don't need to do it" and "can make do".  At
least one of the points of these reviews is to identify and squash cases
of "technically don't need to do it" and "can make do" when it revolves
around doing something half-assed that should be done properly instead.

The cxgb? drivers use the kernel firmware load mechanism.  I'm fine with
that.

The Mellanox drivers use a custom, user space tool.  I *was* fine with
that, when it was a one off thing.  Now you are talking about doing the
same thing, and it's going to become a two off thing.  That makes me
somewhat less OK with either one.

Upfitting IPoIB to pass through the netstack eeprom stuff should be
almost trivial to do.

1) Add eeprom_get and eeprom_set routines to struct ib_device
2) In driver, set eeprom_get and eeprom_set in ib_device
3) In IPoIB, copy eeprom_get and eeprom_set from ib_device to ethtool ops

Everything else is done in user space.  You can use ethtool, or you
could write a custom tool to do the trick.  Mellanox could do the same
thing and modify mstflint to just pass the work to the ethool entry
points while still retaining all of their FS/IMG/GUID/MAC/PSID magic in
mstflint.  For your case, if all you need to do is to write a new file
to the eeprom, you should be able to do this:

cat <binary-image> | ethtool --change-eeprom DEVNAME magic MAGIC offset
0 length `wc -c <binary-image>`

and the eeprom is written with the new data.  If you need to do special
things, like Mellanox, in terms of recovering burned data like GIDs or
MACs or whatever, then you can write a tool that does that, but leaves
the EEPROM banging to the driver and just passes the buffers around
using the ethtool interface.

Really, it doesn't look hard at all, and it sounds much better than
continuing with everybody writing their own tool and reinventing the wheel.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Jason Gunthorpe]

On Thu, May 05, 2016 at 02:57:01PM -0400, Doug Ledford wrote:
 
> and the eeprom is written with the new data.  If you need to do special
> things, like Mellanox, in terms of recovering burned data like GIDs
> or

The 'eeprom' and device firmware are very different things. hfi1 has
both, and uses request_firmware too.

I've never heard of a driver using ethtool eeprom to deal with nv
firmware like mlx has. AFAIK there is no kernel convention for that
stuff. It is more common for storage than network drivers.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

On 05/05/2016 03:20 PM, Jason Gunthorpe wrote:
> On Thu, May 05, 2016 at 02:57:01PM -0400, Doug Ledford wrote:
>  
>> and the eeprom is written with the new data.  If you need to do special
>> things, like Mellanox, in terms of recovering burned data like GIDs
>> or
> 
> The 'eeprom' and device firmware are very different things. hfi1 has
> both, and uses request_firmware too.
> 
> I've never heard of a driver using ethtool eeprom to deal with nv
> firmware like mlx has.

There's no reason it couldn't.  Since you can pass offset and length
parameters and write things in multiple chunks, you can actually set up
access to eeprom, nv ram, and firmware all through the one interface
simply by defining the start/stop points of each to be at specific, well
known locations for your device.  Then your routine could write to the
correct device based upon the location.

And in truth, whether you are talking about firmware, some sort of
eeprom, or nv ram, it's all the same basic principle: read from or write
to this non-volatile memory of arbitrary size (ok, depending on the
device, firmware might be different...it might not be non-volatile, or
there might be a non-volatile version for boot and a running copy that
can be downloaded by the kernel where the running copy and the boot time
non-volatile copy are not necessarily the same).  Only the the nitty
gritty of the access varies based upon the device in question.  Aside
from making sure to get the right device, user space couldn't care less
about the distinction of the hardware type used to implement it.

> AFAIK there is no kernel convention for that
> stuff. It is more common for storage than network drivers.

You're right, it is more common for storage devices (raid hardware in
particular). And a lot of that hardware doesn't have a good means for
doing this.  Different vendors have rolled their own solutions (Dell for
instance uses their own kernel driver to allow updated the firmware on
all of the devices they ship, including raid storage controllers,
onboard Ethernet and PXE booters, etc).  But since we have a netdevice
on everything, I see no reason not to use the eeprom routines already
present.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Jason Gunthorpe]

On Thu, May 05, 2016 at 03:32:32PM -0400, Doug Ledford wrote:
> On 05/05/2016 03:20 PM, Jason Gunthorpe wrote:
> > On Thu, May 05, 2016 at 02:57:01PM -0400, Doug Ledford wrote:
> >  
> >> and the eeprom is written with the new data.  If you need to do special
> >> things, like Mellanox, in terms of recovering burned data like GIDs
> >> or
> > 
> > The 'eeprom' and device firmware are very different things. hfi1 has
> > both, and uses request_firmware too.
> > 
> > I've never heard of a driver using ethtool eeprom to deal with nv
> > firmware like mlx has.
> 
> There's no reason it couldn't.  Since you can pass offset and length
> parameters and write things in multiple chunks, you can actually set up
> access to eeprom, nv ram, and firmware all through the one interface
> simply by defining the start/stop points of each to be at specific, well
> known locations for your device.

Well, sort of.

firmware write tends to be super-critical, doing it wrong can often
mean the card is bricked. eg some devices require good firmware to
start the PCI-E at all.

This means the firmware write process needs to be bomb-proof and all
competent vendors provide a user space program that does all necesary
checks. Using the latest version of that program is always a good idea
:)

I would be strongly against moving that sort of complexity into the
kernel.

In turn this means users will never have a uniform user space
experience, like 'cat | ethtool' - because that will not include the
checks.

Further, the very worst thing we could do is create a situation where
a new kernel driver is required to do a firmware update (eg because we
decided to move the checks into the kernel), and worse, potentially
the new driver won't load on old firmware or old kernels. IIRC mlx had
some problems like this once.

>From that view, I think, if it can be don entirely via resource0, then
that is what vendors should do, there is no value in a common API for
firmware nv writing.

ethtool eeprom exists as simple debugging/helper tool that should
really never be used by end users. It is reasonble to duplicate it for
eeprom like things, and AFAIK those uses cannot truely brick the
hardware.

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Ira Weiny]

On Thu, May 05, 2016 at 02:38:58PM -0600, Jason Gunthorpe wrote:
> On Thu, May 05, 2016 at 03:32:32PM -0400, Doug Ledford wrote:
> > On 05/05/2016 03:20 PM, Jason Gunthorpe wrote:
> > > On Thu, May 05, 2016 at 02:57:01PM -0400, Doug Ledford wrote:
> > >  
> > >> and the eeprom is written with the new data.  If you need to do special
> > >> things, like Mellanox, in terms of recovering burned data like GIDs
> > >> or
> > > 
> > > The 'eeprom' and device firmware are very different things. hfi1 has
> > > both, and uses request_firmware too.
> > > 
> > > I've never heard of a driver using ethtool eeprom to deal with nv
> > > firmware like mlx has.
> > 
> > There's no reason it couldn't.  Since you can pass offset and length
> > parameters and write things in multiple chunks, you can actually set up
> > access to eeprom, nv ram, and firmware all through the one interface
> > simply by defining the start/stop points of each to be at specific, well
> > known locations for your device.
> 
> Well, sort of.
> 
> firmware write tends to be super-critical, doing it wrong can often
> mean the card is bricked. eg some devices require good firmware to
> start the PCI-E at all.

The firmware for hfi1 is already done with the kernel standard firmware
functions.

I think we will need Mellanox to weigh in on their firmware update but I
suspect that it is a critical operation which needs to be handled very
carefully.

> 
> This means the firmware write process needs to be bomb-proof and all
> competent vendors provide a user space program that does all necesary
> checks. Using the latest version of that program is always a good idea
> :)
> 
> I would be strongly against moving that sort of complexity into the
> kernel.
> 
> In turn this means users will never have a uniform user space
> experience, like 'cat | ethtool' - because that will not include the
> checks.
> 
> Further, the very worst thing we could do is create a situation where
> a new kernel driver is required to do a firmware update (eg because we
> decided to move the checks into the kernel), and worse, potentially
> the new driver won't load on old firmware or old kernels. IIRC mlx had
> some problems like this once.
> 
> From that view, I think, if it can be don entirely via resource0, then
> that is what vendors should do, there is no value in a common API for
> firmware nv writing.
> 
> ethtool eeprom exists as simple debugging/helper tool that should
> really never be used by end users. It is reasonble to duplicate it for
> eeprom like things, and AFAIK those uses cannot truely brick the
> hardware.

The eeprom update for hfi1 should be a rare operation.  resource0 gives us
enough access to do this in the field but with very carefully crafted
instructions and/or tools.  This keeps the kernel simple yet gives us access
without requiring users to change their kernels.  The only exception would be a
lock to tell the driver and hardware we are accessing registers.

Perhaps this is as simple as calling open on a debugfs file then we
automatically know when the process has gone away?

All of this can be done with _very_ simple kernel code which really never has
to change while maintaining a very high degree of flexibility.

Ira

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

Re: [RFC] Proposal to address hfi1 UI and EPROM devices

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1871 bytes --]

On Thu, May 05, 2016 at 12:08:43PM -0600, Jason Gunthorpe wrote:
> On Thu, May 05, 2016 at 03:39:32PM +0300, Leon Romanovsky wrote:
> 
> > > I think the message really should be that if your driver contains uAPI
> > > changes those should be in separate patches that are clearly identified. So
> > > if you have a driver that is developed off-list initially, instead of just
> > > breaking it up into chunks for submission add another step.
> > > 
> > > Something like this:
> > > 1) Submit patch series which break-ups internally developed code
> > > 2) Submit patch series with separated out uAPI code
> > > 3) Submit patch that makes the build go-live
> 
> Yes. Perhaps even submit #2 after getting #1 mainlined.
> 
> Make it easy to find the important/controversial things and the review
> process will work much better for everyone. Buring stuff in a monster
> patch is just going to stretch it out.
> 
> > > These can all be submitted together, but with the patches broken up like
> > > this reviewers can target uAPI code more easily.
>  
> > At the end, there is no point of accepting (1) without finished review
> > of (2 and 3). Right now all patch series already have such internal separation
> > in a slightly different order.
> 
> Eh?
> 
> Drivers should be able to stand alone without dedicated uapis
> (excluding the udata stuff).

And they are, however the point is a little bit different. There is no
meaning in internal feature implementation without access through
verbs interface.

> 
> For instance, the HFI1 driver is as functional as any other RDMA
> driver without it's cdev, eeprom, debug and sysfs uAPIs. Those are all
> value add features that do not impact the driver's ability to operate
> as an RDMA device.

Let's close for them PSM and verbs interfaces and see how it is
operable.

> 
> Jason

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]