Messages in this thread | | | Date | Tue, 27 Jun 2023 08:39:12 +0200 | Subject | Re: [PATCH v2 4/5] nvme-fc: Make initial connect attempt synchronous | From | Hannes Reinecke <> |
| |
On 6/27/23 08:18, Daniel Wagner wrote: > On Mon, Jun 26, 2023 at 02:33:18PM +0300, Dan Carpenter wrote: >>> @@ -2943,6 +2943,8 @@ nvme_fc_create_io_queues(struct nvme_fc_ctrl *ctrl) >>> /* force put free routine to ignore io queues */ >>> ctrl->ctrl.tagset = NULL; >>> >>> + if (ret > 0) >>> + ret = -EIO; >> >> All these checks for ret > 0 make me unhappy. I don't understand how >> they are a part of the commit. > > We have two types of error message types in the nvme subsystem. The negative > values are the usual ones and positive ones are nvme protocol errors. > > For example if the authentication fails because of invalid credentials when > doing the authentication nvmf_connect_admin_queue() will return a value of > NVME_SC_AUTH_REQUIRED. This is also the value which gets propagated to this > point here. The problem is any positive error code is interpreted as a valid > pointer later in the code, which results in a crash. > >> I have tried to look at the context and I think maybe you are working >> around the fact that qla_nvme_ls_req() returns QLA_FUNCTION_FAILED on >> error. > > The auth blktests are exercising the error path here and that's why I added > this check. BTW, we already use in other places, this is not completely new in > this subsystem. > >> Also the qla_nvme_ls_req() function EINVAL on error. I just wrote a >> commit message saying that none of the callers cared but I missed that >> apparently gets returned to nvme_fc_init_ctrl(). :/ >> https://lore.kernel.org/all/49866d28-4cfe-47b0-842b-78f110e61aab@moroto.mountain/ > > Thank! > >> Let's just fix qla_nvme_ls_req() instead of working around it here. >> >> And let's add a WARN_ON_ONCE() somewhere to prevent future bugs. > > This makes sense for the driver APIs. Though for the core nvme subsystem this > needs to be discusses/redesigned how to handle the protocol errors first. > I would stick with the 'normal' nvme syntax of having negative errors as internal errors (ie errnos), '0' for no error, and positive numbers as NVMe protocol errors. As such I would also advocate to not map NVMe protocol errors onto error numbers but rather fix the callers to not do a pointer conversion.
Cheers,
Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman
| |