>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.
you are correct
>
>> >
>> >> + 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;
>
>
I think I've tried your suggestion and it didn't went well, I'll retry it.
>>
>> >> + }
>> >> + } 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.
it all depends on the hypervisor's implementation below, I think loop over can be used but I'll need to check
>
>> >
>> >> +
>> >> + 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.
ok
>
>> >
>> >> + 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.
weird, I'll check it out.
>
>> >
>> >> + printk(KERN_ERR "failed to init vSMP version op\n");
>> >> + return -1;
>> >> + }
>> >> +
>> >> + return vsmp_register_sysfs_group(&version_raw_attr);
>> >> +}
Thanks,
Eial
