Re: [RFC] staging/vSMP: new driver

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Mar 17, 2022 at 01:41:30PM +0000, Czerwacki, Eial wrote:
> >> +     printk(KERN_INFO "mapping bar 0: [0x%llx,0x%llx]\n",
> >> +            mapped_bars[0].start, mapped_bars[0].start + mapped_bars[0].len);
> >> +     cfg_addr = ioremap(mapped_bars[0].start, mapped_bars[0].len);
> >> +
> >> +     if (!cfg_addr) {
> >
> >Delete the blank link between the call and the error checking.  They are
> >part of the same thing.
> did you meant line?
> 

What I mean is that it's like paragraphs.

	cfg_addr = ioremap(mapped_bars[0].start, mapped_bars[0].len);
	if (!cfg_addr)
		return -ENOMEM;

The call and the check go together.

> >
> >> +             printk(KERN_ERR
> >> +                    "failed to map vSMP pci controller at %x:%x.%x, exiting.\n",
> >> +                    vsmp_bus, vsmp_dev, vsmp_func);
> >> +             return -1;
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +/* exported functions */
> >> +
> >> +/**
> >> + * read a value from a specfic register in the vSMP's device config space
> >> + */
> >> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)
> >
> >
> >Use u64 when 64 bit types are intended.
> >
> >> +{
> >> +     unsigned long ret_val;
> >
> >
> >u64 ret_val;
> sure
> 
> >
> >> +
> >> +     switch (type) {
> >> +     case VSMP_CTL_REG_SIZE_8BIT:
> >> +             ret_val = readb(cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_16BIT:
> >> +             ret_val = readw(cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_32BIT:
> >> +             ret_val = readl(cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_64BIT:
> >> +             ret_val = readq(cfg_addr + reg);
> >> +             break;
> >> +
> >> +     default:
> >> +             printk(KERN_ERR "unsupported reg size type %d.\n", type);
> >> +             ret_val = (unsigned long)(-1);
> >
> >
> >64 bit types.  "ret_val = -1ULL;"
> >
> >
> >> +     }
> >> +
> >> +     dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x of %d bits\n",
> >> +             __func__, ret_val, reg, (type + 1) * 8);
> >> +     return ret_val;
> >> +}
> >> +EXPORT_SYMBOL_GPL(vsmp_read_reg_from_cfg);
> >> +
> >> +/**
> >> + * write a value to a specfic register in the vSMP's device config space
> >> + */
> >> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
> >> +                        enum reg_size_type type)
> >> +{
> >> +     switch (type) {
> >> +     case VSMP_CTL_REG_SIZE_8BIT:
> >> +             writeb((unsigned char)value, cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_16BIT:
> >> +             writew((unsigned short)value, cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_32BIT:
> >> +             writel((unsigned int)value, cfg_addr + reg);
> >> +             break;
> >> +
> >> +     case VSMP_CTL_REG_SIZE_64BIT:
> >> +             writeq(value, cfg_addr + reg);
> >> +             break;
> >> +
> >> +     default:
> >> +             printk(KERN_ERR "unsupported reg size type %d.\n", type);
> >> +     }
> >> +
> >> +     dev_dbg(&vsmp_dev_obj->dev, "%s: wrote 0x%x to reg 0x%x of %u bits\n",
> >> +             __func__, reg, reg, (type + 1) * 8);
> >> +}
> >> +EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
> >> +
> >> +/**
> >> + * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
> >> + */
> >> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
> >
> >This is unsigned int but it returns zero or negative error codes.
> >
> >Create a separate function for "halt_on_null".  It will be cleaner that
> >way.
> I'll refractor this code properly.
> 
> >
> >> +                                  char *out, unsigned int len,
> >> +                                  bool halt_on_null)
> >> +{
> >> +     unsigned char __iomem *buff;
> >> +
> >> +     BUG_ON(!mapped_bars[bar].start);
> >> +     BUG_ON(ARRAY_SIZE(mapped_bars) <= bar);
> >> +     BUG_ON((offset + len) > mapped_bars[bar].len);
> >> +
> >> +     buff = ioremap(mapped_bars[bar].start + offset, len);
> >> +     if (!buff)
> >> +             return -ENOMEM;
> >> +
> >> +     if (halt_on_null) {
> >> +             int i;
> >> +             unsigned char c;
> >> +
> >> +             for (i = 0; i < len; i++) {
> >> +                     c = ioread8(&buff[i]);
> >> +                     if (!c)
> >> +                             break;
> >> +                     out[i] = c;
> >
> >Copy the NUL terminator to out[i] before the break.
> >
> should I memset out to zero instead?

Right now you're relying on the caller to set the NUL terminator and
that's ugly.  Just do:

		for (i = 0; i < len; i++) {
			c = ioread8(&buff[i]);
			out[i] = c;
			if (!c)
				break;


> 
> >> +             }
> >> +     } else
> >> +             memcpy_fromio(out, buff, len);
> >> +
> >> +     iounmap(buff);
> >> +
> >> +     return 0;
> >> +}

[ snip ]

> >> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> >> +                         struct bin_attribute *bin_attr,
> >> +                         char *buf, loff_t off, size_t count)
> >> +{
> >> +     ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
> >> +                                              vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
> >> +                                              buf, off, count);
> >
> >Don't put functions which can fail in the declaration block.
> >
> >        ret_val = vsmp_generic_buff_read(&op, 0,
> >                                         vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
> >                                         buf, off, count);
> >
> can you explain what is the reason behind this recommendation?
> 

That's just my own preference, not really a rule.  I stole that
guideline from someone else.  I forget who.  But it's a fact that the
declaration block is more bug prone than other code.  (Very clean in
static analysis results).  Plus it means that you put a blank line
between the call and the check which I do not like (again just my
preference).


> >> +
> >> +     if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))
> >
> >Don't test for specific error codes.
> >
> >        if (ret_val < 0)
> >                return ret_val;
> >
> >
> now I understand, there was a patchset which fixes for all the -1 and the specific ret_val testing, I see it
> was misplaced at some point of the work.
> thanks for pointing it out
> 
> >> +         buf[ret_val++] = '\n';
> >> +
> >> +     return ret_val;
> >> +}
> >> +
> >> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
> >> +
> >> +/**
> >> + * retrive str in order to determine the proper length.
> >> + * this is the best way to maintain backwards compatibility with all
> >> + * vSMP versions.
> >> + */
> >> +static int get_version_len(void)
> >> +{
> >> +     unsigned reg;
> >> +     char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);
> >
> >VERSION_MAX_LEN is 524288 bytes.  That's too long.  Create a small
> >buffer and loop over it until you find the NUL terminator.  Why does
> >this need to be ATOMIC, are we holding a lock?  Hopefully if we can fix
> >the length the __GFP_NOWARN will be unnecessary.
> I'm not sure it is possible to loopover, I'll take that into consideration.
> in this flow, only one access is possible, so I assume GFP_ATOMIC is not needed
> 

This get_version_len() is cray cray...  It needs to loop.

> >
> >> +
> >> +     if (!version_str)
> >> +             return -1;
> >> +
> >> +     reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> >> +     memset(version_str, 0, VERSION_MAX_LEN);
> >> +
> >> +     if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
> >> +             printk(KERN_ERR "failed to read buffer from bar\n");
> >> +             return -1;
> >> +     }
> >> +
> >> +     version_raw_attr.size = strlen(version_str);
> >
> >get_version_len() should return the length instead of setting a global.
> do you mean version_raw_attr.size = get_version_len()?
> 

Yes, but with negative error codes.

> >
> >> +     kfree(version_str);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +/**
> >> + * register the version sysfs entry
> >> + */
> >> +int sysfs_register_version_cb(void)
> >> +{
> >> +     if (get_version_len()) {
> >> +             printk(KERN_ERR "failed to init vSMP version module\n");
> >> +             return -1;
> >> +     }
> >> +
> >> +     if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {
> >
> >This if statement is reversed so this code can never work.
> not sure I follow, can you explain?
> 

The vsmp_init_op() function returns zero on success and this code treats
success as failure.

> >
> >> +             printk(KERN_ERR "failed to init vSMP version op\n");
> >> +             return -1;
> >> +     }
> >> +
> >> +     return vsmp_register_sysfs_group(&version_raw_attr);
> >> +}

regards,
dan carpenter