Messages in this thread | | | From | Bart Van Assche <> | Subject | Re: [PATCH 5/7] scsi: ufs: Refactor descriptor read for write | Date | Mon, 4 Jun 2018 08:40:44 +0000 |
| |
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.
| |