* [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails @ 2015-01-19 14:43 ` Dan Carpenter 0 siblings, 0 replies; 7+ messages in thread From: Dan Carpenter @ 2015-01-19 14:43 UTC (permalink / raw) To: kernel-janitors In the current code, if blk_mq_alloc_tag_set() fails then it returns zero (success) instead of preserving the error code. The caller is not expecting that and the kernel could be left in an inconsistent state. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- Static analysis stuff. Not tested. diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c index cb529e9..2ff5efc 100644 --- a/drivers/block/nvme-core.c +++ b/drivers/block/nvme-core.c @@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev) dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE; dev->tagset.driver_data = dev; - if (blk_mq_alloc_tag_set(&dev->tagset)) + res = blk_mq_alloc_tag_set(&dev->tagset); + if (res) goto out; id_ns = mem; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails @ 2015-01-19 14:43 ` Dan Carpenter 0 siblings, 0 replies; 7+ messages in thread From: Dan Carpenter @ 2015-01-19 14:43 UTC (permalink / raw) In the current code, if blk_mq_alloc_tag_set() fails then it returns zero (success) instead of preserving the error code. The caller is not expecting that and the kernel could be left in an inconsistent state. Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com> --- Static analysis stuff. Not tested. diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c index cb529e9..2ff5efc 100644 --- a/drivers/block/nvme-core.c +++ b/drivers/block/nvme-core.c @@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev) dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE; dev->tagset.driver_data = dev; - if (blk_mq_alloc_tag_set(&dev->tagset)) + res = blk_mq_alloc_tag_set(&dev->tagset); + if (res) goto out; id_ns = mem; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails 2015-01-19 14:43 ` Dan Carpenter (?) @ 2015-01-22 4:40 ` Jens Axboe 2015-01-22 16:48 ` Keith Busch -1 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2015-01-22 4:40 UTC (permalink / raw) On 01/19/2015 07:43 AM, Dan Carpenter wrote: > In the current code, if blk_mq_alloc_tag_set() fails then it returns > zero (success) instead of preserving the error code. The caller is not > expecting that and the kernel could be left in an inconsistent state. > > Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com> Looks good to me, Keith, could you ack/review it? Leaving it below... > Static analysis stuff. Not tested. > > diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c > index cb529e9..2ff5efc 100644 > --- a/drivers/block/nvme-core.c > +++ b/drivers/block/nvme-core.c > @@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev) > dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE; > dev->tagset.driver_data = dev; > > - if (blk_mq_alloc_tag_set(&dev->tagset)) > + res = blk_mq_alloc_tag_set(&dev->tagset); > + if (res) > goto out; > > id_ns = mem; > > _______________________________________________ > Linux-nvme mailing list > Linux-nvme at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-nvme > -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails 2015-01-22 4:40 ` Jens Axboe @ 2015-01-22 16:48 ` Keith Busch 2015-01-22 22:46 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Keith Busch @ 2015-01-22 16:48 UTC (permalink / raw) On Wed, 21 Jan 2015, Jens Axboe wrote: > On 01/19/2015 07:43 AM, Dan Carpenter wrote: >> In the current code, if blk_mq_alloc_tag_set() fails then it returns >> zero (success) instead of preserving the error code. The caller is not >> expecting that and the kernel could be left in an inconsistent state. >> >> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com> > > Looks good to me, Keith, could you ack/review it? Leaving it below... Should we bail on the device if tagset allocation fails? If so, the patch is good, but I thought it was a concious descision to not return error here so the controller can be managed. Capabilities would be limited and a failure here probably means there's a bigger problem, so I'm okay either way. >> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c >> index cb529e9..2ff5efc 100644 >> --- a/drivers/block/nvme-core.c >> +++ b/drivers/block/nvme-core.c >> @@ -2152,7 +2152,8 @@ static int nvme_dev_add(struct nvme_dev *dev) >> dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE; >> dev->tagset.driver_data = dev; >> >> - if (blk_mq_alloc_tag_set(&dev->tagset)) >> + res = blk_mq_alloc_tag_set(&dev->tagset); >> + if (res) >> goto out; >> >> id_ns = mem; ^ permalink raw reply [flat|nested] 7+ messages in thread
* [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails 2015-01-22 16:48 ` Keith Busch @ 2015-01-22 22:46 ` Jens Axboe 2015-01-22 23:11 ` Keith Busch 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2015-01-22 22:46 UTC (permalink / raw) On 01/22/2015 09:48 AM, Keith Busch wrote: > On Wed, 21 Jan 2015, Jens Axboe wrote: >> On 01/19/2015 07:43 AM, Dan Carpenter wrote: >>> In the current code, if blk_mq_alloc_tag_set() fails then it returns >>> zero (success) instead of preserving the error code. The caller is not >>> expecting that and the kernel could be left in an inconsistent state. >>> >>> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com> >> >> Looks good to me, Keith, could you ack/review it? Leaving it below... > > Should we bail on the device if tagset allocation fails? If so, the patch > is good, but I thought it was a concious descision to not return error > here so the controller can be managed. Capabilities would be limited > and a failure here probably means there's a bigger problem, so I'm okay > either way. That's a good point, you could still send admin IO through the ioctls even if this fails. Looking at the rest of the function, the error handling is a bit strange. If we fail the nvme_identify(), we'll just march on. If the next works, we're good, we return success. But if the failed one happens to be the last one, we return error. So we need to clean it up a bit regardless. Question is, what errors constitute general failure, and which ones we want to allow. If the rationale is wanting to still access the device and do admin IO, then none of them should be hard failures. But they should be reported. I can imagine cases where the device is mostly screwed and you just want to get the driver loaded successfully to reset/format/fw-update (actually I don't need to imagine, I've been in those situations!). -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails 2015-01-22 22:46 ` Jens Axboe @ 2015-01-22 23:11 ` Keith Busch 2015-01-22 23:15 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Keith Busch @ 2015-01-22 23:11 UTC (permalink / raw) On Thu, 22 Jan 2015, Jens Axboe wrote: > On 01/22/2015 09:48 AM, Keith Busch wrote: >> Should we bail on the device if tagset allocation fails? If so, the patch >> is good, but I thought it was a concious descision to not return error >> here so the controller can be managed. Capabilities would be limited >> and a failure here probably means there's a bigger problem, so I'm okay >> either way. > > That's a good point, you could still send admin IO through the ioctls > even if this fails. Looking at the rest of the function, the error > handling is a bit strange. If we fail the nvme_identify(), we'll just > march on. If the next works, we're good, we return success. But if the > failed one happens to be the last one, we return error. It's a little simpler than that. The return value is unconditionally cleared to 0 at the end so we always return success if the first nvme_identify() was successful. I wouldn't trust a controller that couldn't identify itself, but the remaining commands are for namespace identification which can legitimately return errors. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails 2015-01-22 23:11 ` Keith Busch @ 2015-01-22 23:15 ` Jens Axboe 0 siblings, 0 replies; 7+ messages in thread From: Jens Axboe @ 2015-01-22 23:15 UTC (permalink / raw) On 01/22/2015 04:11 PM, Keith Busch wrote: > On Thu, 22 Jan 2015, Jens Axboe wrote: >> On 01/22/2015 09:48 AM, Keith Busch wrote: >>> Should we bail on the device if tagset allocation fails? If so, the >>> patch >>> is good, but I thought it was a concious descision to not return error >>> here so the controller can be managed. Capabilities would be limited >>> and a failure here probably means there's a bigger problem, so I'm okay >>> either way. >> >> That's a good point, you could still send admin IO through the ioctls >> even if this fails. Looking at the rest of the function, the error >> handling is a bit strange. If we fail the nvme_identify(), we'll just >> march on. If the next works, we're good, we return success. But if the >> failed one happens to be the last one, we return error. > > It's a little simpler than that. The return value is unconditionally > cleared to 0 at the end so we always return success if the first > nvme_identify() was successful. I wouldn't trust a controller that > couldn't identify itself, but the remaining commands are for namespace > identification which can legitimately return errors. Ah correct, I missed that unconditional clear. I think it looks fine then. -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-01-22 23:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-19 14:43 [patch] NVMe: return an error code if blk_mq_alloc_tag_set() fails Dan Carpenter 2015-01-19 14:43 ` Dan Carpenter 2015-01-22 4:40 ` Jens Axboe 2015-01-22 16:48 ` Keith Busch 2015-01-22 22:46 ` Jens Axboe 2015-01-22 23:11 ` Keith Busch 2015-01-22 23:15 ` Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.