Re: [RFC 1/3] nvme: Return -ENOMEM when kzalloc fails

[Joel Granados <j.granados@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 1676 bytes --]

On Thu, Oct 27, 2022 at 10:26:57AM -0600, Keith Busch wrote:
> On Thu, Oct 27, 2022 at 05:57:22PM +0200, Joel Granados wrote:
> > In nvme_init_non_mdts_limits function we were returning 0 when kzalloc
> > failed. This patch corrects this behavior and return -ENOMEM
> > Fixes: 5befc7c26e5a ("nvme: implement non-mdts command limits")
> 
> I'm pretty sure I had this returning 0 on purpose. We can proceed
> without this optional structure.

Looking a bit more into this I see a couple of things:
1. If we fail in the kzalloc, ctrl->max_discard_segments and
   ctrl->max_zeroes_sectors do not get set but we return 0. On the other
   hand if nvme_submit_sync_cmd fails, ctrl->max_discard_segments and
   ctrl->max_zeroes_sectors still do not get set but we forward
   nvme_submit_sync_cmd's error code.
   * Shouldn't we just set ret to 0 before returning regardless of what
     happens in nvme_submit_sync_cmd?
   * If we are returning 0 no matter what, shouldn't this function be
     void?

2. The function nvme_scan_work will not rescan the controler if
   nvme_init_non_mdts_limits has a return value that is less than zero.
   * Should we remove the check in nvme_scan_work to let the scan move
     forward regardless of the return value from
     nvme_init_non_mdts_limits?

3. I think what makes more sense is to:
  a. Make nvme_init_non_mdts_limits a void function
  b. Move the warning of failing to read mdts limits inside
     nvme_init_non_mdts_limits
  c. Allow the scan work to move forward after executing
     nvme_init_non_mdts_limits

What do you think? I can spin up a patch for this if it makes sense.

Best

Joel


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]