* Re: [PATCH V6 11/11] nvme: pci: support nested EH [not found] ` <20180517022030.GB21959@localhost.localdomain> @ 2018-05-17 8:41 ` Christoph Hellwig 2018-05-17 14:20 ` Keith Busch 0 siblings, 1 reply; 5+ messages in thread From: Christoph Hellwig @ 2018-05-17 8:41 UTC (permalink / raw) To: Keith Busch Cc: Ming Lei, Keith Busch, Jens Axboe, Laurence Oberman, Sagi Grimberg, James Smart, linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, Jianchao Wang, Christoph Hellwig, Johannes Thumshirn, bhelgaas, linux-pci, arjan On Wed, May 16, 2018 at 08:20:31PM -0600, Keith Busch wrote: > On Thu, May 17, 2018 at 07:10:59AM +0800, Ming Lei wrote: > > All simulation in block/011 may happen in reality. > > If this test actually simulates reality, then the following one line > patch (plus explanation for why) would be a real "fix" as this is very > successful in passing block/011. :) > > --- > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 1faa32cd07da..dcc5746304c4 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -2118,6 +2118,12 @@ static int nvme_pci_enable(struct nvme_dev *dev) > > if (pci_enable_device_mem(pdev)) > return result; > + /* > + * blktests block/011 disables the device without the driver knowing. > + * We'll just enable the device twice to get the enable_cnt > 1 > + * so that the test's disabling does absolutely nothing. > + */ > + pci_enable_device_mem(pdev); Heh. But yes, this test and the PCI "enable" interface in sysfs look horribly wrong. pci_disable_device/pci_enable_device aren't something we can just do underneath the driver. Even if injecting the lowlevel config space manipulations done by it might be useful and a good test the Linux state ones are just killing the driver. The enable attribute appears to have been added by Arjan for the Xorg driver. I think if we have a driver bound to the device we should not allow it. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V6 11/11] nvme: pci: support nested EH 2018-05-17 8:41 ` [PATCH V6 11/11] nvme: pci: support nested EH Christoph Hellwig @ 2018-05-17 14:20 ` Keith Busch 2018-05-17 14:23 ` Johannes Thumshirn 0 siblings, 1 reply; 5+ messages in thread From: Keith Busch @ 2018-05-17 14:20 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, linux-block@vger.kernel.org, Laurence Oberman, Sagi Grimberg, linux-pci, James Smart, linux-nvme@lists.infradead.org, Ming Lei, Keith Busch, Jianchao Wang, Johannes Thumshirn, bhelgaas, arjan On Thu, May 17, 2018 at 10:41:29AM +0200, Christoph Hellwig wrote: > On Wed, May 16, 2018 at 08:20:31PM -0600, Keith Busch wrote: > > On Thu, May 17, 2018 at 07:10:59AM +0800, Ming Lei wrote: > > > All simulation in block/011 may happen in reality. > > > > If this test actually simulates reality, then the following one line > > patch (plus explanation for why) would be a real "fix" as this is very > > successful in passing block/011. :) > > > > --- > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > > index 1faa32cd07da..dcc5746304c4 100644 > > --- a/drivers/nvme/host/pci.c > > +++ b/drivers/nvme/host/pci.c > > @@ -2118,6 +2118,12 @@ static int nvme_pci_enable(struct nvme_dev *dev) > > > > if (pci_enable_device_mem(pdev)) > > return result; > > + /* > > + * blktests block/011 disables the device without the driver knowing. > > + * We'll just enable the device twice to get the enable_cnt > 1 > > + * so that the test's disabling does absolutely nothing. > > + */ > > + pci_enable_device_mem(pdev); > > Heh. But yes, this test and the PCI "enable" interface in sysfs look > horribly wrong. pci_disable_device/pci_enable_device aren't something we > can just do underneath the driver. Even if injecting the lowlevel > config space manipulations done by it might be useful and a good test > the Linux state ones are just killing the driver. Yes, I'm totally fine with injecting errors into the config space, but for goodness sakes, don't fuck with the internal kernel structures out from under drivers using them. My suggestion is to use 'setpci' to disable the device if you want to inject this scenario. That way you get the desired broken device scenario you want to test, but struct pci_dev hasn't been altered. > The enable attribute appears to have been added by Arjan for the > Xorg driver. I think if we have a driver bound to the device we > should not allow it. Agreed. Alternatively possibly call the driver's reset_preparei/done callbacks. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V6 11/11] nvme: pci: support nested EH 2018-05-17 14:20 ` Keith Busch @ 2018-05-17 14:23 ` Johannes Thumshirn 2018-05-18 16:28 ` Keith Busch 0 siblings, 1 reply; 5+ messages in thread From: Johannes Thumshirn @ 2018-05-17 14:23 UTC (permalink / raw) To: Keith Busch Cc: Christoph Hellwig, Ming Lei, Keith Busch, Jens Axboe, Laurence Oberman, Sagi Grimberg, James Smart, linux-nvme@lists.infradead.org, linux-block@vger.kernel.org, Jianchao Wang, bhelgaas, linux-pci, arjan On Thu, May 17, 2018 at 08:20:51AM -0600, Keith Busch wrote: > > Heh. But yes, this test and the PCI "enable" interface in sysfs look > > horribly wrong. pci_disable_device/pci_enable_device aren't something we > > can just do underneath the driver. Even if injecting the lowlevel > > config space manipulations done by it might be useful and a good test > > the Linux state ones are just killing the driver. > > Yes, I'm totally fine with injecting errors into the config space, but > for goodness sakes, don't fuck with the internal kernel structures out > from under drivers using them. > > My suggestion is to use 'setpci' to disable the device if you want to > inject this scenario. That way you get the desired broken device > scenario you want to test, but struct pci_dev hasn't been altered. > > > The enable attribute appears to have been added by Arjan for the > > Xorg driver. I think if we have a driver bound to the device we > > should not allow it. > > Agreed. Alternatively possibly call the driver's reset_preparei/done > callbacks. Exactly, but as long as we can issue the reset via sysfs the test-case is still valid. -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V6 11/11] nvme: pci: support nested EH 2018-05-17 14:23 ` Johannes Thumshirn @ 2018-05-18 16:28 ` Keith Busch 2018-05-22 7:35 ` Johannes Thumshirn 0 siblings, 1 reply; 5+ messages in thread From: Keith Busch @ 2018-05-18 16:28 UTC (permalink / raw) To: Johannes Thumshirn Cc: Jens Axboe, linux-block@vger.kernel.org, Laurence Oberman, Sagi Grimberg, linux-pci, James Smart, linux-nvme@lists.infradead.org, Ming Lei, Keith Busch, Jianchao Wang, bhelgaas, arjan, Christoph Hellwig On Thu, May 17, 2018 at 04:23:45PM +0200, Johannes Thumshirn wrote: > > Agreed. Alternatively possibly call the driver's reset_preparei/done > > callbacks. > > Exactly, but as long as we can issue the reset via sysfs the test-case > is still valid. I disagree the test case is valid. The test writes '0' to the pci-sysfs 'enable', but the driver also disables the pci device as part of resetting, which is a perfectly reasonable thing for a driver to do. If the timing of the test's loop happens to write '0' right after the driver disabled the device that it owns, a 'write error' on that sysfs write occurs, and blktests then incorrectly claims the test failed. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V6 11/11] nvme: pci: support nested EH 2018-05-18 16:28 ` Keith Busch @ 2018-05-22 7:35 ` Johannes Thumshirn 0 siblings, 0 replies; 5+ messages in thread From: Johannes Thumshirn @ 2018-05-22 7:35 UTC (permalink / raw) To: Keith Busch Cc: Jens Axboe, linux-block@vger.kernel.org, Laurence Oberman, Sagi Grimberg, linux-pci, James Smart, linux-nvme@lists.infradead.org, Ming Lei, Keith Busch, Jianchao Wang, bhelgaas, arjan, Christoph Hellwig On Fri, May 18, 2018 at 10:28:04AM -0600, Keith Busch wrote: > On Thu, May 17, 2018 at 04:23:45PM +0200, Johannes Thumshirn wrote: > > > Agreed. Alternatively possibly call the driver's reset_preparei/done > > > callbacks. > > = > > Exactly, but as long as we can issue the reset via sysfs the test-case > > is still valid. > = > I disagree the test case is valid. The test writes '0' to the > pci-sysfs 'enable', but the driver also disables the pci device as part > of resetting, which is a perfectly reasonable thing for a driver to do. > = > If the timing of the test's loop happens to write '0' right after the > driver disabled the device that it owns, a 'write error' on that sysfs > write occurs, and blktests then incorrectly claims the test failed. Hmm, ok that's a valid point. But seeing you have sent a patch for blktests anyways I think it's gone now. -- = Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N=FCrnberg GF: Felix Imend=F6rffer, Jane Smithard, Graham Norton HRB 21284 (AG N=FCrnberg) Key fingerprint =3D EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-22 7:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180516040313.13596-1-ming.lei@redhat.com>
[not found] ` <20180516040313.13596-12-ming.lei@redhat.com>
[not found] ` <20180516141242.GA20119@localhost.localdomain>
[not found] ` <20180516231058.GB28727@ming.t460p>
[not found] ` <20180517022030.GB21959@localhost.localdomain>
2018-05-17 8:41 ` [PATCH V6 11/11] nvme: pci: support nested EH Christoph Hellwig
2018-05-17 14:20 ` Keith Busch
2018-05-17 14:23 ` Johannes Thumshirn
2018-05-18 16:28 ` Keith Busch
2018-05-22 7:35 ` Johannes Thumshirn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).