On Wed, Dec 11, 2024 at 09:30:37AM +0000, John Garry wrote:
> On 10/12/2024 19:47, Keith Busch wrote:
> > +static int nvme_query_fdp_granularity(struct nvme_ctrl *ctrl,
> > + struct nvme_ns_info *info, u8 fdp_idx)
> > +{
> > + struct nvme_fdp_config_log hdr, *h;
> > + struct nvme_fdp_config_desc *desc;
> > + size_t size = sizeof(hdr);
> > + int i, n, ret;
> > + void *log;
> > +
> > + ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> > + NVME_CSI_NVM, &hdr, size, 0, info->endgid);
> > + if (ret) {
> > + dev_warn(ctrl->device,
> > + "FDP configs log header status:0x%x endgid:%x\n", ret,
> > + info->endgid);
>
> About endgid, I guess that there is a good reason but sometimes "0x" is
> prefixed for hex prints and sometimes not. Maybe no prefix is used when we
> know that the variable is to hold a value from a HW register / memory
> structure - I don't know.
%d for endgid is probably a better choice.
> further nitpicking: And ret holds a kernel error code - the driver seems
> inconsistent for printing this. Sometimes it's %d and sometimes 0x%x.
It's either an -errno or an nvme status. "%x" is easier to decode if
it's an nvme status, which is probably the more interesting case to
debug.
> > + return ret;
> > + }
> > +
> > + size = le32_to_cpu(hdr.sze);
> > + h = kzalloc(size, GFP_KERNEL);
> > + if (!h) {
> > + dev_warn(ctrl->device,
> > + "failed to allocate %lu bytes for FDP config log\n",
> > + size);
>
> do we normally print ENOMEM messages? I see that the bytes is printed, but I
> assume that this is a sane value (of little note).
I suppose not.
> > + return -ENOMEM;
> > + }
> > +
> > + ret = nvme_get_log_lsi(ctrl, 0, NVME_LOG_FDP_CONFIGS, 0,
> > + NVME_CSI_NVM, h, size, 0, info->endgid);
> > + if (ret) {
> > + dev_warn(ctrl->device,
> > + "FDP configs log status:0x%x endgid:%x\n", ret,
> > + info->endgid);
> > + goto out;
> > + }
> > +
> > + n = le16_to_cpu(h->numfdpc) + 1;
> > + if (fdp_idx > n) {
> > + dev_warn(ctrl->device, "FDP index:%d out of range:%d\n",
> > + fdp_idx, n);
> > + /* Proceed without registering FDP streams */> + ret = 0;
>
> nit: maybe you want to be explicit, but logically ret is already 0
Yeah, we know its zero already, but there are static analyisis tools
that think returning without setting an error return code was a mistake,
and that we really meant to return something else like -EINVAL. We
definitely want to return 0 here, so this setting exists only to prevent
future "help".
