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

Groups > linux.kernel > #1667249 > unrolled thread

[RFC PATCH 00/13] Switchtec NTB Support

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:40 +0200
    [RFC PATCH 09/13] switchtec_ntb: add link management Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:40 +0200
    [RFC PATCH 04/13] switchtec: add link event notifier block Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:40 +0200
      Re: [RFC PATCH 04/13] switchtec: add link event notifier block Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-17 07:20 +0200
        Re: [RFC PATCH 04/13] switchtec: add link event notifier block Logan Gunthorpe <logang@deltatee.com> - 2017-06-17 18:30 +0200
    [RFC PATCH 06/13] switchtec_ntb: initialize hardware for memory windows Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:50 +0200
      Re: [RFC PATCH 06/13] switchtec_ntb: initialize hardware for memory  windows Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-17 07:20 +0200
        Re: [RFC PATCH 06/13] switchtec_ntb: initialize hardware for memory  windows Logan Gunthorpe <logang@deltatee.com> - 2017-06-17 18:40 +0200
          Re: [RFC PATCH 06/13] switchtec_ntb: initialize hardware for memory  windows Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-18 02:40 +0200
    [RFC PATCH 13/13] switchtec_ntb: update switchtec documentation with notes for ntb Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:50 +0200
    [RFC PATCH 02/13] switchtec: export class symbol for use in upper layer driver Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:50 +0200
      Re: [RFC PATCH 02/13] switchtec: export class symbol for use in  upper layer driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-17 07:20 +0200
        Re: [RFC PATCH 02/13] switchtec: export class symbol for use in upper  layer driver Logan Gunthorpe <logang@deltatee.com> - 2017-06-17 18:20 +0200
    [RFC PATCH 08/13] switchtec_ntb: add skeleton ntb driver Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:50 +0200
    [RFC PATCH 05/13] switchtec_ntb: introduce initial ntb driver Logan Gunthorpe <logang@deltatee.com> - 2017-06-15 22:50 +0200
    RE: [RFC PATCH 00/13] Switchtec NTB Support "Allen Hubbe" <Allen.Hubbe@dell.com> - 2017-06-16 16:00 +0200
      Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-16 16:20 +0200
        RE: [RFC PATCH 00/13] Switchtec NTB Support "Allen Hubbe" <Allen.Hubbe@dell.com> - 2017-06-16 17:40 +0200
          Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-16 18:50 +0200
            Re: [RFC PATCH 00/13] Switchtec NTB Support Serge Semin <fancer.lancer@gmail.com> - 2017-06-16 19:40 +0200
            RE: [RFC PATCH 00/13] Switchtec NTB Support "Allen Hubbe" <Allen.Hubbe@dell.com> - 2017-06-16 20:10 +0200
              Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-16 21:20 +0200
        Re: [RFC PATCH 00/13] Switchtec NTB Support Serge Semin <fancer.lancer@gmail.com> - 2017-06-16 18:40 +0200
          Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-16 19:10 +0200
            Re: [RFC PATCH 00/13] Switchtec NTB Support Serge Semin <fancer.lancer@gmail.com> - 2017-06-16 20:40 +0200
              Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-16 21:40 +0200
                Re: [RFC PATCH 00/13] Switchtec NTB Support Serge Semin <fancer.lancer@gmail.com> - 2017-06-16 22:30 +0200
                  Re: [RFC PATCH 00/13] Switchtec NTB Support 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org> - 2017-06-17 07:20 +0200
                    Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-17 18:20 +0200
                    Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-17 18:20 +0200
                    Re: [RFC PATCH 00/13] Switchtec NTB Support Jon Mason <jdmason@kudzu.us> - 2017-06-19 21:20 +0200
    Re: [RFC PATCH 00/13] Switchtec NTB Support Jon Mason <jdmason@kudzu.us> - 2017-06-19 22:10 +0200
      Re: [RFC PATCH 00/13] Switchtec NTB Support Logan Gunthorpe <logang@deltatee.com> - 2017-06-19 22:30 +0200
        Re: [RFC PATCH 00/13] Switchtec NTB Support Jon Mason <jdmason@kudzu.us> - 2017-06-19 23:10 +0200

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

#1667952

From: Logan Gunthorpe
> On 16/06/17 09:34 AM, Allen Hubbe wrote:
> > In code review, I really only have found minor nits.  Overall, the driver looks good.
> 
> Great, thanks for such a quick review!
> 
> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * 50ms).  This looks like a thread
> context, so it could involve the scheduler for the delay instead of spinning for up to 50s before
> bailing.
> 
> Good point. If I were to change this to msleep_interruptible would that
> be acceptable?

I would be satisfied.

> 
> > There are a few instances like this:
> >> +	dev_dbg(&stdev->dev, "%s\n", __func__);
> 
> > Where the printing of __func__ could be controlled by dyndbg=+pf.  The debug message could be more
> useful.
> 
> Ok, I'll change that.
> 
> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the mask bits is protected by a
> spinlock.  Elsewhere, you noted that the db bits are shared between all ports, so the db bitset is
> chopped up to be shared between the ports.  Is the db mask also shared, and how is the spinlock
> sufficient for synchronizing access to the mask bits between multiple ports?
> 
> Well, there are 64 doorbells that are shared between ports but each port
> has it's own in and out registers for the doorbells. So triggering
> doorbell one on one port's ODB actually triggers it on every ports IDB.
> So these are shared only in the sense that each port needs to know which
> dbs it cares about. Seeing each port has their own registers they don't
> have to worry about synchronization.
> 
> The mask is only protected by a spin lock seeing multiple callers of
> db_set_mask and db_clr_mask on the same port may step on each others
> toes. So if two processes try to mask different bits they both must get
> masked in the end and therefore some kind of synchronization must be
> involved.

Thanks for clearing that up.  Now I understand, each port has its own independent set of mask bits.  So, while the doorbell numbers are assigned globally, the registers themselves are per port.  For the mask bits, the mask behavior only affects the local port.

> 
> > The IDT switch also does not have hardware scratchpads.  Could the code you wrote for emulated
> scratchpads be made into shared library code for ntb drivers?  Also, some ntb clients may not need
> scratchpad support.  If it is not natively supported by a driver, can the emulated scratchpad support
> be an optional feature?
> 
> Hmm, interesting idea. A few pieces could possibly be made common but it
> depends mostly on hardware having the resources to make use of it.
> Switchtec has extra LUT memory windows that made this possible. Unless
> you object I'm inclined to leave it as is and I'd be happy to work with
> the IDT folks to create a common solution in the future.

Alright.  I'll leave it to you to find and reconcile common functionalities of the drivers.  What about making spad emulation optional?

> 
> Logan

There was a comment on irc.oftc.net #ntb wishing for patch v2 to be fewer patches.  Something like, 1/2: prep the existing switch driver, 2/2: introduce the ntb driver.

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

#1668012

On 16/06/17 12:08 PM, Allen Hubbe wrote:
> Alright.  I'll leave it to you to find and reconcile common functionalities of the drivers.  What about making spad emulation optional?

Ok.

I don't see the point of making spad emulation optional. Who would want
to disable it and what would be the benefit?

Logan

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

#1667889

Hello Logan.
Thanks for the new hardware driver. It's really great to see NTB subsystem
being developed.

New NTB API is going to be merged to mainline kernel within next features
merge window, so it's really recommended to use that API for new hardware.
Could you please rabase your driver on top of the tree?
https://github.com/jonmason/ntb.git

> See what is staged in https://github.com/jonmason/ntb.git ntb-next, with
> the addition of multi-peer support by Serge.  It would be good at this
> stage to understand whether the api changes there would also support the
> Switchtec driver, and what if anything must change, or be planned to change,
> to support the Switchtec driver.

My impression is that, there is no need for new NTB API changes, since it
was specifically designed for new type of multi-port switches with
additional message registers, which is exactly supported by switchtec
device.

> The Switchtec hardware supports doorbells, memory windows and messages.
> Seeing there is no native scratchpad support, 128 spads are emulated
> through the use of a pre-setup memory window.

According to the NTB philosophy, it's not good to have any hardware
emulation within hardware driver. Hardware driver must reflect the only
hardware abilities, nothing else. Could you please get rid of Scratchpad
emulation and add messaging as new NTB API has got a proper callback
functions for it?
Hmmm, I haven't seen the actual code (see my last comment), but
according to my impression of API, it's impossible to have memory window
accessed while NTB link is down, but Scratchpads still can be used.
In this case, if you got Scratchpads emulated over memory windows,
you must have got NTB link enabled before NTB device is registered, which
makes ntb_link_* methods kind of being useless unless Switchtec hardware
supports NTB link getting up/down for individual memory windows...

> configurable set of memory windows. While the hardware supports more
> than 2 partitions, this driver only supports the first two seeing
> the current NTB API only supports two hosts.

New NTB API is updated so to have access to any of peer ports. IDT driver
has got a special translation table to access peer functionality just by
providing an index to corresponding API callback. You can use it as
reference to have Switchtec driver accordingly altered. It would be vastly
useful to have one more multi-port hardware driver in the tree.

> switchtec: move structure definitions into a common header
> switchtec: export class symbol for use in upper layer driver
> switchtec: add ntb hardware register definitions
> switchtec: add link event notifier block
> switchtec_ntb: introduce initial ntb driver
> switchtec_ntb: initialize hardware for memory windows
> switchtec_ntb: initialize hardware for doorbells and messages
> switchtec_ntb: add skeleton ntb driver
> switchtec_ntb: add link management
> switchtec_ntb: implement doorbell registers
> switchtec_ntb: implement scratchpad registers
> switchtec_ntb: add memory window support
> switchtec_ntb: update switchtec documentation with notes for ntb

If I'm not mistaken, these patches can be combined the way so to have
just two big functionally split patches:
1) NTB: Microsemt Switchtec switch management driver alterations for NTB
2) NTB: Add Microsemi Switchtec PCIe-switches support
It would really simplify the review. Could you please combine them?


Thanks for submitting the patches. We really appreciate this and looking
forward to have it completely reviewed and added to the kernel tree.

Regards
-Sergey

On Fri, Jun 16, 2017 at 08:09:55AM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> 
> 
> On 16/06/17 07:53 AM, Allen Hubbe wrote:
> > See what is staged in https://github.com/jonmason/ntb.git ntb-next, with the addition of multi-peer support by Serge.  It would be good at this stage to understand whether the api changes there would also support the Switchtec driver, and what if anything must change, or be planned to change, to support the Switchtec driver.
> 
> Ah, yes I had seen that patchset some time ago but I wasn't aware of
> it's status or that it was queued up in ntb-next. I think it will be no
> problem to reconcile with the switchtec driver and I'll rebase onto
> ntb-next for the next posting of the patch set. However, I *may* save
> full multi-host switchtec support for a follow up submission. My initial
> impression is the new API will support the switchtec hardware well.
> 
> Thanks,
> 
> Logan

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

#1667906

On 16/06/17 10:33 AM, Serge Semin wrote:
> New NTB API is going to be merged to mainline kernel within next features
> merge window, so it's really recommended to use that API for new hardware.
> Could you please rabase your driver on top of the tree?
> https://github.com/jonmason/ntb.git

Yes, Allen's already pointed this out. I'll be sure to fix it up before
a final submission.

> According to the NTB philosophy, it's not good to have any hardware
> emulation within hardware driver. Hardware driver must reflect the only
> hardware abilities, nothing else. Could you please get rid of Scratchpad
> emulation and add messaging as new NTB API has got a proper callback
> functions for it?

I disagree completely. Practicality trumps philosophy in every case. I
need emulated scratchpads for ntb_transport to work and I'm not getting
rid of it (thus breaking things that work well) just because of your
philosophical beliefs.

> Hmmm, I haven't seen the actual code (see my last comment), but
> according to my impression of API, it's impossible to have memory window
> accessed while NTB link is down, but Scratchpads still can be used.
> In this case, if you got Scratchpads emulated over memory windows,
> you must have got NTB link enabled before NTB device is registered, which
> makes ntb_link_* methods kind of being useless unless Switchtec hardware
> supports NTB link getting up/down for individual memory windows...

Nothing in-kernel actually uses the peer's scratchpads while the link is
down and all clients seem specifically designed to wait until the link
event to set them. So I think you're either wrong about that rule or we
should change the rule going forward.

I'm not sure what you're referring to about the link stuff; as
implemented, our link management works just fine.

> New NTB API is updated so to have access to any of peer ports. IDT driver
> has got a special translation table to access peer functionality just by
> providing an index to corresponding API callback. You can use it as
> reference to have Switchtec driver accordingly altered. It would be vastly
> useful to have one more multi-port hardware driver in the tree.

Yes, I expect we will get there eventually, it doesn't sound like much
work. However, it's client support that's really going to make this
interesting and worthwhile. That seems like the real challenge right now.

> If I'm not mistaken, these patches can be combined the way so to have
> just two big functionally split patches:
> 1) NTB: Microsemt Switchtec switch management driver alterations for NTB
> 2) NTB: Add Microsemi Switchtec PCIe-switches support
> It would really simplify the review. Could you please combine them?

Seems like every time I make a submission, someone either wants patches
to be smaller and split up more or bigger and combined. I happen to
agree with the people who prefer smaller patches and I think these
provide good chunks for reviewers to look at. So, no, I'm not going to
change this. Feel free to apply the patches to a git tree or view it on
our github branch if you want to see the code combined.

Thanks,

Logan

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

#1668007

On Fri, Jun 16, 2017 at 11:08:52AM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> 
> 
> On 16/06/17 10:33 AM, Serge Semin wrote:
> > New NTB API is going to be merged to mainline kernel within next features
> > merge window, so it's really recommended to use that API for new hardware.
> > Could you please rabase your driver on top of the tree?
> > https://github.com/jonmason/ntb.git
> 
> Yes, Allen's already pointed this out. I'll be sure to fix it up before
> a final submission.
> 
> > According to the NTB philosophy, it's not good to have any hardware
> > emulation within hardware driver. Hardware driver must reflect the only
> > hardware abilities, nothing else. Could you please get rid of Scratchpad
> > emulation and add messaging as new NTB API has got a proper callback
> > functions for it?
> 
> I disagree completely. Practicality trumps philosophy in every case. I
> need emulated scratchpads for ntb_transport to work and I'm not getting
> rid of it (thus breaking things that work well) just because of your
> philosophical beliefs.
> 

It's the way the NTB API was created for, to have set of functions to access
NTB devices in the similar way. These aren't my beliefs, it's the way it was
created. I agree it can be optional, but it shouldn't be made as the basics
of the driver. It is called NTB "hardware" driver after all, not "emulating" or
"abstracting" driver.
ntb_transport could work without Scratchpads, if it's properly altered to
use NTB messaging. This should be the way to make things compatible, but not
making the hardware driver suitable for just one ntb_transport.

> > Hmmm, I haven't seen the actual code (see my last comment), but
> > according to my impression of API, it's impossible to have memory window
> > accessed while NTB link is down, but Scratchpads still can be used.
> > In this case, if you got Scratchpads emulated over memory windows,
> > you must have got NTB link enabled before NTB device is registered, which
> > makes ntb_link_* methods kind of being useless unless Switchtec hardware
> > supports NTB link getting up/down for individual memory windows...
> 
> Nothing in-kernel actually uses the peer's scratchpads while the link is
> down and all clients seem specifically designed to wait until the link
> event to set them. So I think you're either wrong about that rule or we
> should change the rule going forward.
> 
> I'm not sure what you're referring to about the link stuff; as
> implemented, our link management works just fine.
> 
> > New NTB API is updated so to have access to any of peer ports. IDT driver
> > has got a special translation table to access peer functionality just by
> > providing an index to corresponding API callback. You can use it as
> > reference to have Switchtec driver accordingly altered. It would be vastly
> > useful to have one more multi-port hardware driver in the tree.
> 
> Yes, I expect we will get there eventually, it doesn't sound like much
> work. However, it's client support that's really going to make this
> interesting and worthwhile. That seems like the real challenge right now.
> 
> > If I'm not mistaken, these patches can be combined the way so to have
> > just two big functionally split patches:
> > 1) NTB: Microsemy Switchtec switch management driver alterations for NTB
> > 2) NTB: Add Microsemi Switchtec PCIe-switches support
> > It would really simplify the review. Could you please combine them?
> 
> Seems like every time I make a submission, someone either wants patches
> to be smaller and split up more or bigger and combined. I happen to
> agree with the people who prefer smaller patches and I think these
> provide good chunks for reviewers to look at. So, no, I'm not going to
> change this. Feel free to apply the patches to a git tree or view it on
> our github branch if you want to see the code combined.

It's not like my whim or something, but the way it's usually done.
https://kernelnewbies.org/PatchPhilosophy

Cite from there:
"Each patch should group changes into a logical sequence. Bug fixes must
come first in the patchset, then new features. This is because we need to be
able to backport bug fixes to older kernels, and they should not depend on
new features."

You grouped the patches in according to your logical view or development
progress (I don't know for sure), but it's not obvious for reviewers.
From my perspective your new Microsemi Switchtec NTB driver is just one
feature. I don't know who would think differently so to split the solid
driver up for review. Switchtec management driver alteration might be the
same - just one fix. It's much easier for you to have your commits squashed,
than for me to look at your git tree, than get back to your patchset looking
for a necessary peace of patch and commenting it there.

Regards,
-Sergey

> 
> Thanks,
> 
> Logan
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/883bdb76-972c-7de9-0208-2d0933f192d4%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

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

#1668028

On 16/06/17 12:38 PM, Serge Semin wrote:
> On Fri, Jun 16, 2017 at 11:08:52AM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> It's the way the NTB API was created for, to have set of functions to access
> NTB devices in the similar way. These aren't my beliefs, it's the way it was
> created. I agree it can be optional, but it shouldn't be made as the basics
> of the driver. It is called NTB "hardware" driver after all, not "emulating" or
> "abstracting" driver.

Just more philosophy. You haven't given any good reason to remove the
functionality. Vague references to the way things were created aren't
compelling arguments. Better to cite code and point out actual problems.

> ntb_transport could work without Scratchpads, if it's properly altered to
> use NTB messaging. This should be the way to make things compatible, but not
> making the hardware driver suitable for just one ntb_transport.

Ok, well when all the NTB clients no longer require using scratchpads
and we can all abide by the rule that clients must function without
them. Then, I'll remove the emulation. Until then, it stays.

> It's not like my whim or something, but the way it's usually done.
> https://kernelnewbies.org/PatchPhilosophy

> Cite from there:
> "Each patch should group changes into a logical sequence. Bug fixes must
> come first in the patchset, then new features. This is because we need to be
> able to backport bug fixes to older kernels, and they should not depend on
> new features."

You should probably read that again because it doesn't actually support
your point (in fact it's saying something quite unrelated). It is also
probably a good idea to read the rest of the seciton you cite:

"The idea here is that you should break changes up in such a way that it
will be easy to review."

"When creating a new feature patchset, you may need to break up your
changes into multiple commits. "

"Clean up patches that are over 200 lines long are discouraged, because
they are hard to review. Break those patches up into smaller patches. "

Also, to quote Greg Kroah-Hartman from my last series[1]:

"That's one big patch to review, would you want to do that?

Can you break it up into smaller parts?"

> You grouped the patches in according to your logical view or development
> progress (I don't know for sure), but it's not obvious for reviewers.
> From my perspective your new Microsemi Switchtec NTB driver is just one
> feature. I don't know who would think differently so to split the solid
> driver up for review. Switchtec management driver alteration might be the
> same - just one fix. It's much easier for you to have your commits squashed,
> than for me to look at your git tree, than get back to your patchset looking
> for a necessary peace of patch and commenting it there.

Well you're free to think that but, in my experience, your opinion
differs significantly from the rest of the kernel community which I
personally agree with.

Now, if you'd like to actually review the code I'd be happy to address
any concerns you find. I won't be responding to any more philosophical
arguments or bike-shedding over the format of the patch.

Logan

[1] https://lkml.org/lkml/2017/1/31/637

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

#1668044

On Fri, Jun 16, 2017 at 01:34:59PM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> 
> 
> On 16/06/17 12:38 PM, Serge Semin wrote:
> > On Fri, Jun 16, 2017 at 11:08:52AM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> > It's the way the NTB API was created for, to have set of functions to access
> > NTB devices in the similar way. These aren't my beliefs, it's the way it was
> > created. I agree it can be optional, but it shouldn't be made as the basics
> > of the driver. It is called NTB "hardware" driver after all, not "emulating" or
> > "abstracting" driver.
> 
> Just more philosophy. You haven't given any good reason to remove the
> functionality. Vague references to the way things were created aren't
> compelling arguments. Better to cite code and point out actual problems.
> 

Actual problem is the design of your driver. Of course you can disagree as much as
you want.

> > ntb_transport could work without Scratchpads, if it's properly altered to
> > use NTB messaging. This should be the way to make things compatible, but not
> > making the hardware driver suitable for just one ntb_transport.
> 
> Ok, well when all the NTB clients no longer require using scratchpads
> and we can all abide by the rule that clients must function without
> them. Then, I'll remove the emulation. Until then, it stays.
> 
> > It's not like my whim or something, but the way it's usually done.
> > https://kernelnewbies.org/PatchPhilosophy
> 
> > Cite from there:
> > "Each patch should group changes into a logical sequence. Bug fixes must
> > come first in the patchset, then new features. This is because we need to be
> > able to backport bug fixes to older kernels, and they should not depend on
> > new features."
> 
> You should probably read that again because it doesn't actually support
> your point (in fact it's saying something quite unrelated). It is also
> probably a good idea to read the rest of the seciton you cite:
> 
> "The idea here is that you should break changes up in such a way that it
> will be easy to review."
> 
> "When creating a new feature patchset, you may need to break up your
> changes into multiple commits. "
> 
> "Clean up patches that are over 200 lines long are discouraged, because
> they are hard to review. Break those patches up into smaller patches. "
> 

This doesn't prove your way of splitting patchset is correct, but supports
my point. As well as the sentence about the logical sentence in addition
to the thing about easy review.

> Also, to quote Greg Kroah-Hartman from my last series[1]:
> 
> "That's one big patch to review, would you want to do that?
> 
> Can you break it up into smaller parts?"
> 
> > You grouped the patches in according to your logical view or development
> > progress (I don't know for sure), but it's not obvious for reviewers.
> > From my perspective your new Microsemi Switchtec NTB driver is just one
> > feature. I don't know who would think differently so to split the solid
> > driver up for review. Switchtec management driver alteration might be the
> > same - just one fix. It's much easier for you to have your commits squashed,
> > than for me to look at your git tree, than get back to your patchset looking
> > for a necessary peace of patch and commenting it there.
> 
> Well you're free to think that but, in my experience, your opinion
> differs significantly from the rest of the kernel community which I
> personally agree with.
> 

And your quotation doesn't prove you are right. Greg asked you to split at
least the documentation. He had point to ask it, since it's logically correct.
You wasn't arguing with him, was you? But in this case you have sent the
set of incremental patches of your own code, so I don't see how it can be
easier for review, than a combined text.

> Now, if you'd like to actually review the code I'd be happy to address
> any concerns you find. I won't be responding to any more philosophical
> arguments or bike-shedding over the format of the patch.
> 

I don't want to review a patchset, which isn't properly formated.

> Logan
> 
> [1] https://lkml.org/lkml/2017/1/31/637
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/33b6c321-c0af-7340-8e8e-e929a00005c7%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

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

#1668173

On Fri, Jun 16, 2017 at 11:21:00PM +0300, Serge Semin wrote:
> On Fri, Jun 16, 2017 at 01:34:59PM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> > Now, if you'd like to actually review the code I'd be happy to address
> > any concerns you find. I won't be responding to any more philosophical
> > arguments or bike-shedding over the format of the patch.
> > 
> 
> I don't want to review a patchset, which isn't properly formated.

Ah, but the patchset does seem to properly formatted.  At least it's
easy for me to review as-published, while a much smaller number of
patches, making much larger individual patches, would be much much
harder to review.

But what do I know...

Oh wait, I review more kernel patches than anyone else :)

Logan, given that you need to rebase these on the "new" ntb api (and why
the hell is that tree on github? We can't take kernel git pulls from
github), is it worth reviewing this patch series as-is, or do you want
us to wait?

thanks,

greg k-h

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

#1668359

On 16/06/17 11:09 PM, 'Greg Kroah-Hartman' wrote:
> Ah, but the patchset does seem to properly formatted.  At least it's
> easy for me to review as-published, while a much smaller number of
> patches, making much larger individual patches, would be much much
> harder to review.
> 
> But what do I know...
> 
> Oh wait, I review more kernel patches than anyone else :)

Thanks Greg.

> Logan, given that you need to rebase these on the "new" ntb api (and why
> the hell is that tree on github? We can't take kernel git pulls from
> github), is it worth reviewing this patch series as-is, or do you want
> us to wait?

I think initial review at this time will still be useful. I don't expect
the patchset will change _that_ much.

Logan

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

#1668360

On 16/06/17 11:09 PM, 'Greg Kroah-Hartman' wrote:
> Ah, but the patchset does seem to properly formatted.  At least it's
> easy for me to review as-published, while a much smaller number of
> patches, making much larger individual patches, would be much much
> harder to review.
> 
> But what do I know...
> 
> Oh wait, I review more kernel patches than anyone else :)

Thanks Greg!

> Logan, given that you need to rebase these on the "new" ntb api (and why
> the hell is that tree on github? We can't take kernel git pulls from
> github), is it worth reviewing this patch series as-is, or do you want
> us to wait?

I think review at this time is still appropriate. The new ntb api mostly
just adds indexes for the port so I don't expect things to change too much.

Thanks for the review so far.

Logan

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

#1669787

On Sat, Jun 17, 2017 at 07:09:59AM +0200, 'Greg Kroah-Hartman' wrote:
> On Fri, Jun 16, 2017 at 11:21:00PM +0300, Serge Semin wrote:
> > On Fri, Jun 16, 2017 at 01:34:59PM -0600, Logan Gunthorpe <logang@deltatee.com> wrote:
> > > Now, if you'd like to actually review the code I'd be happy to address
> > > any concerns you find. I won't be responding to any more philosophical
> > > arguments or bike-shedding over the format of the patch.
> > > 
> > 
> > I don't want to review a patchset, which isn't properly formated.
> 
> Ah, but the patchset does seem to properly formatted.  At least it's
> easy for me to review as-published, while a much smaller number of
> patches, making much larger individual patches, would be much much
> harder to review.
> 
> But what do I know...
> 
> Oh wait, I review more kernel patches than anyone else :)
> 
> Logan, given that you need to rebase these on the "new" ntb api (and why
> the hell is that tree on github? We can't take kernel git pulls from
> github), is it worth reviewing this patch series as-is, or do you want

Well, Linus has been taking my pull request from it since v3.12.  He did
call me out initially for requesting it initially, but was amenible
after I GPG signed all of my pull requests (and had a sufficient number
of people he "knew" in my ring).  But all of that has been sorted out
now.

The reason it is on Github is for the Wiki of NTB HW and it's usage
https://github.com/jonmason/ntb/wiki
It's gotten a bit stale, but was very useful back in the v3.12 days :)
(Also, I am using this as a call to update the Wiki!)

Thanks,
Jon

> us to wait?
> 
> thanks,
> 
> greg k-h

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

#1669900

On Thu, Jun 15, 2017 at 02:37:16PM -0600, Logan Gunthorpe wrote:
> Hi,
> 
> This patchset implements Non-Transparent Bridge (NTB) support for the
> Microsemi Switchtec series of switches. We're looking for some
> review from the community at this point but hope to get it upstreamed
> for v4.14.
> 
> Switchtec NTB support is configured over the same function and bar
> as the management endpoint. Thus, the new driver hooks into the
> management driver which we had merged in v4.12. We use the class
> interface API to register an NTB device for every switchtec device
> which supports NTB (not all do).
> 
> The Switchtec hardware supports doorbells, memory windows and messages.
> Seeing there is no native scratchpad support, 128 spads are emulated
> through the use of a pre-setup memory window. The switch has 64
> doorbells which are shared between the two partitions and a
> configurable set of memory windows. While the hardware supports more
> than 2 partitions, this driver only supports the first two seeing
> the current NTB API only supports two hosts.
> 
> The driver has been tested with ntb_netdev and fully passes the
> ntb_test script.
> 
> This patchset is based off of v4.12-rc5 and can be found in this
> git repo:
> 
> https://github.com/sbates130272/linux-p2pmem.git switchtec_ntb

I think this code is of quality enough to go from an RFC to a standard
patch, and we can nit pick it to death there ;-)

Please rebase on ntb-next (which I believe you are already doing), and
resbutmit.

Also, we'll need to decide how to handle the patches to
drivers/pci/switches
We have 2 options here.
1.  We can split it up into 2 separate series, and have Bjorn take the
drivers/pci/switches in his, and I'll take the NTB portions.  The down
side of this is that I'll have to wait for his to get pulled in first,
which will push this back a full kernel release (which based on your
comment above doesn't seem your prefered choice)
2.  I can pull in the drivers/pci/switches patches into my NTB tree,
but I'll want Bjorn's ack on those patches before I'll pull them in.  

I'm thinking that we'll want to keep this series all in one place.
So, #2 sounds like the best option.  But, I need Bjorns $0.02 on this.


FYI, I ran smatch on the patches and got this.  Please correct them in
v2 (or v1 of the non-RFC...however you want to think of it).
drivers/pci/switch/switchtec.c:484 switchtec_dev_read() error: double unlock 'mutex:&stdev->mrpc_mutex'
drivers/pci/switch/switchtec.c:506 switchtec_dev_read() error: double unlock 'mutex:&stdev->mrpc_mutex'
drivers/pci/switch/switchtec.c:513 switchtec_dev_read() warn: inconsistent returns 'mutex:&stdev->mrpc_mutex'.


Thanks,
Jon


> 
> Thanks,
> 
> Logan
> 
> 
> Logan Gunthorpe (13):
>   switchtec: move structure definitions into a common header
>   switchtec: export class symbol for use in upper layer driver
>   switchtec: add ntb hardware register definitions
>   switchtec: add link event notifier block
>   switchtec_ntb: introduce initial ntb driver
>   switchtec_ntb: initialize hardware for memory windows
>   switchtec_ntb: initialize hardware for doorbells and messages
>   switchtec_ntb: add skeleton ntb driver
>   switchtec_ntb: add link management
>   switchtec_ntb: implement doorbell registers
>   switchtec_ntb: implement scratchpad registers
>   switchtec_ntb: add memory window support
>   switchtec_ntb: update switchtec documentation with notes for ntb
> 
>  Documentation/switchtec.txt         |   12 +
>  MAINTAINERS                         |    2 +
>  drivers/ntb/hw/Kconfig              |    1 +
>  drivers/ntb/hw/Makefile             |    1 +
>  drivers/ntb/hw/mscc/Kconfig         |    9 +
>  drivers/ntb/hw/mscc/Makefile        |    1 +
>  drivers/ntb/hw/mscc/switchtec_ntb.c | 1144 +++++++++++++++++++++++++++++++++++
>  drivers/pci/switch/switchtec.c      |  319 ++--------
>  include/linux/ntb.h                 |    3 +
>  include/linux/switchtec.h           |  365 +++++++++++
>  10 files changed, 1601 insertions(+), 256 deletions(-)
>  create mode 100644 drivers/ntb/hw/mscc/Kconfig
>  create mode 100644 drivers/ntb/hw/mscc/Makefile
>  create mode 100644 drivers/ntb/hw/mscc/switchtec_ntb.c
>  create mode 100644 include/linux/switchtec.h
> 
> --
> 2.11.0
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
> To post to this group, send email to linux-ntb@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/20170615203729.9009-1-logang%40deltatee.com.
> For more options, visit https://groups.google.com/d/optout.

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

#1669921

On 19/06/17 02:07 PM, Jon Mason wrote:
> I think this code is of quality enough to go from an RFC to a standard
> patch, and we can nit pick it to death there ;-)

Thanks!

> Please rebase on ntb-next (which I believe you are already doing), and
> resbutmit.

I'll try to get the rebase done and all the feedback so far applied by
the end of the week and resend a v1.

> I'm thinking that we'll want to keep this series all in one place.
> So, #2 sounds like the best option.  But, I need Bjorns $0.02 on this.

I was thinking #2 was the best choice as well but really it's for you
maintainers to decide. And, yes, we'd want to get Bjorn's ack.

> FYI, I ran smatch on the patches and got this.  Please correct them in
> v2 (or v1 of the non-RFC...however you want to think of it).
> drivers/pci/switch/switchtec.c:484 switchtec_dev_read() error: double unlock 'mutex:&stdev->mrpc_mutex'
> drivers/pci/switch/switchtec.c:506 switchtec_dev_read() error: double unlock 'mutex:&stdev->mrpc_mutex'
> drivers/pci/switch/switchtec.c:513 switchtec_dev_read() warn: inconsistent returns 'mutex:&stdev->mrpc_mutex'.

This looks like a false positive to me. The code looks correct. smatch
may have been confused by the fact that the lock is taken by two calls
to the static function 'lock_mutex_and_test_alive'.

This is also part of the switchtec management driver that's already in
the kernel and not part of the NTB related patches I sent.

Logan

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

#1669953

On Mon, Jun 19, 2017 at 4:27 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 19/06/17 02:07 PM, Jon Mason wrote:
>> I think this code is of quality enough to go from an RFC to a standard
>> patch, and we can nit pick it to death there ;-)
>
> Thanks!
>
>> Please rebase on ntb-next (which I believe you are already doing), and
>> resbutmit.
>
> I'll try to get the rebase done and all the feedback so far applied by
> the end of the week and resend a v1.
>
>> I'm thinking that we'll want to keep this series all in one place.
>> So, #2 sounds like the best option.  But, I need Bjorns $0.02 on this.
>
> I was thinking #2 was the best choice as well but really it's for you
> maintainers to decide. And, yes, we'd want to get Bjorn's ack.

Bjorn is a busy guy, but I'm sure he'll get to it soon enough.

>> FYI, I ran smatch on the patches and got this.  Please correct them in
>> v2 (or v1 of the non-RFC...however you want to think of it).
>> drivers/pci/switch/switchtec.c:484 switchtec_dev_read() error: double unlock 'mutex:&stdev->mrpc_mutex'
>> drivers/pci/switch/switchtec.c:506 switchtec_dev_read() error: double unlock 'mutex:&stdev->mrpc_mutex'
>> drivers/pci/switch/switchtec.c:513 switchtec_dev_read() warn: inconsistent returns 'mutex:&stdev->mrpc_mutex'.
>
> This looks like a false positive to me. The code looks correct. smatch
> may have been confused by the fact that the lock is taken by two calls
> to the static function 'lock_mutex_and_test_alive'.
>
> This is also part of the switchtec management driver that's already in
> the kernel and not part of the NTB related patches I sent.

Ah, no problem.  I'm not a master user of smatch, but I do find it
very useful.  So, I do not know of a way to run it against only the
patches being applied, and I'm not sure that is possible to do so.
Either way, I'm sure someone will address that then, given that it is
already in the kernel.

BTW, I don't think I said so, but I'm really excited to have another
piece of NTB HW supported in the kernel.

Thanks,
Jon

>
> Logan

[toc] | [prev] | [standalone]

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

Back to top | Article view | linux.kernel

csiph-web