Re: [PATCH V23 2/3] misc: dcc: Add driver support for Data Capture and Compare unit(DCC)

[Greg Kroah-Hartman]

On Thu, Jun 15, 2023 at 06:13:53PM +0530, Souradeep Chowdhury wrote:
> 
> 
> On 6/15/2023 4:03 PM, Greg Kroah-Hartman wrote:
> > On Thu, May 04, 2023 at 11:36:22PM -0700, Souradeep Chowdhury wrote:
> > > +/**
> > > + * struct dcc_config_entry - configuration information related to each dcc instruction
> > > + * @base:                    Base address of the register to be configured in dcc
> > 
> > Why is this a u32 and not a bigger size?
> 
> Currently only 32 bit register addresses are supported for DCC
> configuration.
> 
> > 
> > > + * @offset:                  Offset to the base address to be configured in dcc
> > > + * @len:                     Length of the address in words to be configured in dcc
> > 
> > What is a "word" here, 16 bits?
> 
> Each word is 4 bytes(32 bits)

See, I guess wrong, you should say what this is :)

> > > + * @loop_cnt:                The number of times to loop on the register address in case
> > > +				of loop instructions
> > > + * @write_val:               The value to be written on the register address in case of
> > > +				write instructions
> > > + * @mask:                    Mask corresponding to the value to be written in case of
> > > +				write instructions
> > > + * @apb_bus:                 Type of bus to be used for the instruction, can be either
> > > +				'apb' or 'ahb'
> > 
> > How can a bool be either "apb" or "ahb"?
> 
> 1 stands for apb and 0 for ahb. Will update the same here.

Why not have an enum?  Will there ever be another "bus"?

> > > +static ssize_t ready_read(struct file *filp, char __user *userbuf,
> > > +			  size_t count, loff_t *ppos)
> > > +{
> > > +	int ret = 0;
> > > +	char *buf;
> > > +	struct dcc_drvdata *drvdata = filp->private_data;
> > > +
> > > +	mutex_lock(&drvdata->mutex);
> > > +
> > > +	if (!is_dcc_enabled(drvdata)) {
> > > +		ret = -EINVAL;
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	if (!FIELD_GET(BIT(1), readl(drvdata->base + dcc_status(drvdata->mem_map_ver))))
> > > +		buf = "Y\n";
> > > +	else
> > > +		buf = "N\n";
> > > +out_unlock:
> > > +	mutex_unlock(&drvdata->mutex);
> > > +
> > > +	if (ret < 0)
> > > +		return -EINVAL;
> > > +	else
> > 
> > You do the "lock, get a value, unlock, do something with the value"
> > thing a bunch, but what prevents the value from changing after the lock
> > happens?  So why is the lock needed at all?
> 
> The lock is used to prevent concurrent accesses of the drv_data when
> scripts are being run from userspace.

How would that matter?  The state can change instantly after the lock is
given up, and then the returned value is now incorrect.  So no need for
a lock at all as you really aren't "protecting" anything, or am I
missing something else?

thanks,

greg k-h