[RFC PATCH 0/5] to add more nvme reset functions

[RFC PATCH 0/5] to add more nvme reset functions

[tsutomu.owa]

Hi Sagi, Christoph, Keith,

This series of patch is to add support for nvme reset functions
described in NVM-Express specification 1.4 "7.3.2 Controller Level Reset" 
(conventional hot reset, link down reset and function level reset)
and "7.3.3 Queue Level" (queue level reset).

This series applies cleanly on top of linux-5.5-rc1. 

I'm still wondoring
	- if it's ok to export functions from drivers/pci.
	- if it would be better not to use CONFIG_NVME_PCI_RESET
	  and/or CONFIG_NVME_QUELE_LEVEL_RESET as these are noisy
	  and these features are based on specification anyway...
	- what is the best way to export nvme_pci_* functions in
	  drivers/nvme/host/pci.c to drivers/nvme/host/core.c.

Any comments would be highly appreciated.

It would be greate if this patch goes into mainline eventually.

thanks in advance.
-- owa

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 0/5] to add more nvme reset functions

[Keith Busch]

On Wed, Dec 11, 2019 at 02:34:15AM +0000, tsutomu.owa@kioxia.com wrote:
> This series of patch is to add support for nvme reset functions
> described in NVM-Express specification 1.4 "7.3.2 Controller Level Reset" 
> (conventional hot reset, link down reset and function level reset)
> and "7.3.3 Queue Level" (queue level reset).

You're not providing any justification for why you want these implemented
in the nvme driver. What issue is this addressing?
 
> I'm still wondoring
> 	- if it's ok to export functions from drivers/pci.

Probably not. They are private for a reason.

> 	- if it would be better not to use CONFIG_NVME_PCI_RESET
> 	  and/or CONFIG_NVME_QUELE_LEVEL_RESET as these are noisy
> 	  and these features are based on specification anyway...

First, if you're going to introduce a Kconfig option, don't split the
patch that actually uses it. Second, don't introduce new kernel config
options for such features in the first place.

> 	- what is the best way to export nvme_pci_* functions in
> 	  drivers/nvme/host/pci.c to drivers/nvme/host/core.c.

Don't export functions that direction. Use ctrl->ops if core needs to
call a transport specific routine.

> Any comments would be highly appreciated.

The majority of this series is at the pci protocol level, and the pci
driver already provides the requested capabilities (but with a more
safe implementation). Let's not reinvent it just because one nvme
transport happens to use the pci bus.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 0/5] to add more nvme reset functions

[Sagi Grimberg]

>> Any comments would be highly appreciated.
> 
> The majority of this series is at the pci protocol level, and the pci
> driver already provides the requested capabilities (but with a more
> safe implementation). Let's not reinvent it just because one nvme
> transport happens to use the pci bus.

This is messy. Everything here needs to live in nvme-pci.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 0/5] to add more nvme reset functions

[Christoph Hellwig]

On Wed, Dec 11, 2019 at 04:50:23PM -0800, Sagi Grimberg wrote:
> 
> > > Any comments would be highly appreciated.
> > 
> > The majority of this series is at the pci protocol level, and the pci
> > driver already provides the requested capabilities (but with a more
> > safe implementation). Let's not reinvent it just because one nvme
> > transport happens to use the pci bus.
> 
> This is messy. Everything here needs to live in nvme-pci.

I really don't see any good reason to have any of this code to be
honest.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [RFC PATCH 0/5] to add more nvme reset functions

[tsutomu.owa]

Hi Keith-san,

Thank you for your reply.

> From: Keith Busch [mailto:kbusch@kernel.org]
> Sent: Thursday, December 12, 2019 1:18 AM

> You're not providing any justification for why you want these implemented
> in the nvme driver. What issue is this addressing?
  Please give me some time to write it in another email.

> First, if you're going to introduce a Kconfig option, don't split the
> patch that actually uses it. Second, don't introduce new kernel config
> options for such features in the first place.
  Ok, I'll remove these Kconfig option.

> > 	- what is the best way to export nvme_pci_* functions in
> > 	  drivers/nvme/host/pci.c to drivers/nvme/host/core.c.
> 
> Don't export functions that direction. Use ctrl->ops if core needs to
> call a transport specific routine.
  Yes, I'll use ctrl-ops if needed.

> > I'm still wondoring
> > 	- if it's ok to export functions from drivers/pci.
> 
> Probably not. They are private for a reason.

> > Any comments would be highly appreciated.
> 
> The majority of this series is at the pci protocol level, and the pci
> driver already provides the requested capabilities (but with a more
> safe implementation). Let's not reinvent it just because one nvme
> transport happens to use the pci bus.
Thank you for the comment.  I'll look into the pci driver again to look for 
functions avilable.  Would you please point out function names good for
conventional hot reset and/or link down reset which can be called within
the kernel if you know.

thanks for your time.
-- owa


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [RFC PATCH 0/5] to add more nvme reset functions

[tsutomu.owa]

Hi Sagi-san,

thank you for your comment.

> From: Sagi Grimberg [mailto:sagi@grimberg.me]
> Sent: Thursday, December 12, 2019 9:50 AM
> This is messy. Everything here needs to live in nvme-pci.
So you mean that those should be in drivers/nvme/host/pci.c at least?

thanks.
-- owa

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [RFC PATCH 0/5] to add more nvme reset functions

[Keith Busch]

On Thu, Dec 12, 2019 at 08:36:27AM +0000, tsutomu.owa@kioxia.com wrote:
> > From: Keith Busch [mailto:kbusch@kernel.org]
> > Sent: Thursday, December 12, 2019 1:18 AM
> > 
> > The majority of this series is at the pci protocol level, and the pci
> > driver already provides the requested capabilities (but with a more
> > safe implementation). Let's not reinvent it just because one nvme
> > transport happens to use the pci bus.
> Thank you for the comment.  I'll look into the pci driver again to look for 
> functions avilable.  Would you please point out function names good for
> conventional hot reset and/or link down reset which can be called within
> the kernel if you know.

The pci level reset that's safe for a user to invoke is through generic
the pci driver's provided 'reset' attribute. That will verify different
types of pci device resets until if finds a supported one. You can reach
it for an nvme device like:

  # echo 1 > /sys/class/nvme/nvmeX/devices/reset

The kernel's entry is defined in drivers/pci/pci-syfs.c, reset_store(). It
calls pci_reset_function().

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme