On Thu, Oct 31, 2019 at 10:13:21AM -0600, Revanth Rajashekar wrote:
> This feature gives the user RW access to any opal table with admin1
> authority. The flags described in the new structure determines if the user
> wants to read/write the data. Flags are checked for valid values in
> order to allow future features to be added to the ioctl.
>
> The user can provide the desired table's UID. Also, the ioctl provides a
> size and offset field and internally will loop data accesses to return
> the full data block. Read overrun is prevented by the initiator's
> sec_send_recv() backend. The ioctl provides a private field with the
> intention to accommodate any future expansions to the ioctl.
>
> Reviewed-by: Jon Derrick <jonathan.derrick@xxxxxxxxx>
> Signed-off-by: Revanth Rajashekar <revanth.rajashekar@xxxxxxxxx>
Looks fine
Reviewed-by: Scott Bauer <sbauer@xxxxxxxxxxxxxx>
> +static int read_table_data(struct opal_dev *dev, void *data)
> +{
> + struct opal_read_write_table *read_tbl = data;
> + int err;
> + size_t off = 0, max_read_size = OPAL_MAX_READ_TABLE;
> + u64 table_len, len;
> + u64 offset = read_tbl->offset, read_size = read_tbl->size - 1;
> + u8 __user *dst;
> +
> + /* len+1: This includes the NULL terminator at the end*/
> + if (dev->prev_d_len > len + 1) {
> + err = -EOVERFLOW;
> + break;
> + }
> +
> + dst = (u8 __user *)(uintptr_t)read_tbl->data;
> + if (copy_to_user(dst + off, dev->prev_data, dev->prev_d_len)) {
> + pr_debug("Error copying data to userspace\n");
> + err = -EFAULT;
> + break;
> + }
> + dev->prev_data = NULL;
If you end up needing to spin a v4 can you please add a comment here reminding me that prev_data in this scenario is not kmalloc'd memory but an offset into the response buffer. I had to go track down why you were not kfree()ing this. I know in a year I'll have forgotten it and will review it again.
> +static int opal_generic_read_write_table(struct opal_dev *dev,
> + struct opal_read_write_table *rw_tbl)
> +{
> + int ret, bit_set;
> +
> + mutex_lock(&dev->dev_lock);
> + setup_opal_dev(dev);
> +
> + bit_set = fls64(rw_tbl->flags) - 1;
Maybe I am missing something obvious but why don't we just use flags as a number? Like instead of bit packing flags we just use it as a number like number 0 is read_table, 1 is write_table, 2 is delete_table, 3 is clear_table etc etc. I don't understand the neccessity for fls-ing some bit field when a number would suffice.
I don't want another revision --it's fine-- I'm just wondering why this method was chosen.
