Re: [PATCH 2/3] coresight: ultrasoc-smb: simplify the code for check to_copy valid

[hejunhao]

On 2023/10/19 21:35, Jonathan Cameron wrote:
> On Thu, 12 Oct 2023 17:47:05 +0800
> Junhao He <hejunhao3@xxxxxxxxxx> wrote:
>
> > We only need to check once when before using the to_copy variable
> > to simplify the code.
> >
> > Signed-off-by: Junhao He <hejunhao3@xxxxxxxxxx>
> I'm not convinced this one is an improvement. Sometimes it's easier to just
> see the individual conditions checked even if we could combine them.
> It's easy to understand we don't copy data if:
> a) We ask for 0 data.
> b) There is 0 data
>
> Less easy to establish that with the extra wrap around code in there
> (even though that has no impact on to_copy if it is 0)
>
> Jonathan

Thanks, I will drop this patch.

Best regards,
Junhao.

> > ---
> >   drivers/hwtracing/coresight/ultrasoc-smb.c | 13 ++++---------
> >   1 file changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/ultrasoc-smb.c b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > index b08a619d1116..e78edc3480ce 100644
> > --- a/drivers/hwtracing/coresight/ultrasoc-smb.c
> > +++ b/drivers/hwtracing/coresight/ultrasoc-smb.c
> > @@ -127,20 +127,15 @@ static ssize_t smb_read(struct file *file, char __user *data, size_t len,
> >   					struct smb_drv_data, miscdev);
> >   	struct smb_data_buffer *sdb = &drvdata->sdb;
> >   	struct device *dev = &drvdata->csdev->dev;
> > -	ssize_t to_copy = 0;
> > -
> > -	if (!len)
> > -		return 0;
> > -
> > -	if (!sdb->data_size)
> > -		return 0;
> > -
> > -	to_copy = min(sdb->data_size, len);
> > +	ssize_t to_copy = min(sdb->data_size, len);
> >   
> >   	/* Copy parts of trace data when read pointer wrap around SMB buffer */
> >   	if (sdb->buf_rdptr + to_copy > sdb->buf_size)
> >   		to_copy = sdb->buf_size - sdb->buf_rdptr;
> >   
> > +	if (!to_copy)
> > +		return 0;
> > +
> >   	if (copy_to_user(data, sdb->buf_base + sdb->buf_rdptr, to_copy)) {
> >   		dev_dbg(dev, "Failed to copy data to user\n");
> >   		return -EFAULT;
> .