On 9/23/25 3:18 AM, Jonathan Cameron wrote:
> On Mon, 22 Sep 2025 14:13:30 -0700
> Dave Jiang <[email protected]> wrote:
>
>> Converting nvdimm_bus_lock/unlock to guard() to clean up usage
>> of gotos for error handling and avoid future mistakes of missed
>> unlock on error paths.
>>
>> Link: https://lore.kernel.org/linux-cxl/[email protected]/
>> Suggested-by: Jonathan Cameron <[email protected]>
>> Signed-off-by: Dave Jiang <[email protected]>
> Hi Dave,
>
> Thanks for looking at this.
>
> Fully agree with Dan about the getting rid of all gotos by end of series.
I'll do that in a different patch for that particular chunk.
>
> A few other things inline. Mostly places where the use of guard()
> opens up low hanging fruit that improves readability (+ shortens code).
>
> This code has a lot of dev_dbg() and some of them are so generic I'm not
> sure they are actually useful (cover a whole set of error paths). Perhaps
> it is worth splitting some of those up, or reducing the paths that trigger
> them as part of this refactor.
> > Jonathan
>
>
<snip>
>
>> diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
>> index 0ccf4a9e523a..d2c2d71e7fe0 100644
>> --- a/drivers/nvdimm/bus.c
>> +++ b/drivers/nvdimm/bus.c
>> static int nvdimm_bus_probe(struct device *dev)
>> @@ -1177,15 +1175,15 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus,
>> struct nvdimm *nvdimm,
>> goto out;
>> }
>>
>> - device_lock(dev);
>> - nvdimm_bus_lock(dev);
>> + guard(device)(dev);
>> + guard(nvdimm_bus)(dev);
>> rc = nd_cmd_clear_to_send(nvdimm_bus, nvdimm, func, buf);
>> if (rc)
>> - goto out_unlock;
>> + goto out;
>>
>> rc = nd_desc->ndctl(nd_desc, nvdimm, cmd, buf, buf_len, &cmd_rc);
>> if (rc < 0)
>> - goto out_unlock;
>> + goto out;
>>
>> if (!nvdimm && cmd == ND_CMD_CLEAR_ERROR && cmd_rc >= 0) {
>> struct nd_cmd_clear_error *clear_err = buf;
>> @@ -1197,9 +1195,6 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus,
>> struct nvdimm *nvdimm,
>> if (copy_to_user(p, buf, buf_len))
>> rc = -EFAULT;
>>
>> -out_unlock:
>> - nvdimm_bus_unlock(dev);
>> - device_unlock(dev);
>> out:
> Hmm. I'm not a fan of gotos that rely on initializing a bunch of pointers to
> NULL
> so fewer labels are used. Will be nice to replace that as well via __free
>
> Going to need a DEFINE_FREE for the vfree that follow these but that looks
> standard to me.
I ended up moving it to kvzalloc() instead. Seems reasonable and give us
zeroing of the buffer as well.
DJ
