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
