On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
> /* Check whether we need temp memory */
> if (param_offset != 0 || param_size < buff_len) {
> - desc_buf = kmalloc(buff_len, GFP_KERNEL);
> + desc_buf = kzalloc(buff_len, GFP_KERNEL);
> if (!desc_buf)
> return -ENOMEM;
> +
> + /* If it's a write, first read the complete descriptor, then
> + * copy in the parts being changed.
> + */
Have you verified this patch with checkpatch? The above comment does not follow
the Linux kernel coding style.
> + if (opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
> + if ((int)param_offset + (int)param_size > buff_len) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = ufshcd_query_descriptor_retry(hba,
> + UPIU_QUERY_OPCODE_READ_DESC,
> + desc_id, desc_index, 0,
> + desc_buf, &buff_len);
> +
> + if (ret) {
> + dev_err(hba->dev,
> + "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
> + __func__, desc_id, desc_index,
> + param_offset, ret);
> +
> + goto out;
> + }
> +
> + memcpy(desc_buf + param_offset, param_buf, param_size);
> + }
The above code is indented deeply. I think that means that this code would become
easier to read if a helper function would be introduced.
Additionally, I think locking is missing from the above code. How else can race
conditions between concurrent writers be prevented?
Bart.
