Re: [PATCH v2] scsi: ufshcd: fix possible unclocked register access

[Subhash Jadavani]

Thanks Kim for the response.

On 2016-10-06 03:28, Kiwoong Kim wrote:
> Hi, Subhash.
>
> Some UFS host controllers may need to call the vendor specific callback
> before and after controlling by clock control framework,
> regardless of whether available clocks are turned on or off.

Are you suggesting to call ufshcd_vops_setup_clocks() 2 times, one 
before the on/off by ufshcd core driver and one after the on/off? If 
yes, then we also have add 3rd argument clarifying if this is PRE_CHANGE 
or POST_CHANGE.

> Is there any special reason to limit to invoke the callback
> only when the clocks are turned on or not?
>
> Besides, the callback is acknowledged from core driver
> because 2nd argument is whether the clocks are turned on or not.
>
> If you have any other idea, please let me know.

This is my suggestion:
1. Add 3rd argument to setup_clocks ops to let the vendor callback 
function know if this is called PRE_CHANGE or POST_CHANGE.
2. If #1 is in place, call setup_clocks 2 times, one with PRE_CHANGE 
argument before making any clock changes in core driver, 2nd with 
POST_CHANGE argument after making the clock changes to core driver.

Let me know if this would work or not.

Thanks,
Subhash

> Thanks
> Regards
>
> > Vendor specific setup_clocks callback may require the clocks managed
> > by ufshcd driver to be ON. So if the vendor specific setup_clocks 
> > callback
> > is called while the required clocks are turned off, it could result 
> > into
> > unclocked register access.
> >
> > To prevent possible unclock register access, this change makes sure 
> > that
> > required clocks remain enabled before calling into vendor specific
> > setup_clocks callback.
> >
> > Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> > ---
> > Changes from v2:
> > * Don't call ufshcd_vops_setup_clocks() again for clock off
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 05c7456..c1a77d3 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -5389,6 +5389,17 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> > *hba, bool on,
> >  	if (!head || list_empty(head))
> >  		goto out;
> >
> > +	/*
> > +	 * vendor specific setup_clocks ops may depend on clocks managed by
> > +	 * this standard driver hence call the vendor specific setup_clocks
> > +	 * before disabling the clocks managed here.
> > +	 */
> > +	if (!on) {
> > +		ret = ufshcd_vops_setup_clocks(hba, on);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	list_for_each_entry(clki, head, list) {
> >  		if (!IS_ERR_OR_NULL(clki->clk)) {
> >  			if (skip_ref_clk && !strcmp(clki->name, "ref_clk"))
> > @@ -5410,7 +5421,16 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> > *hba, bool on,
> >  		}
> >  	}
> >
> > -	ret = ufshcd_vops_setup_clocks(hba, on);
> > +	/*
> > +	 * vendor specific setup_clocks ops may depend on clocks managed by
> > +	 * this standard driver hence call the vendor specific setup_clocks
> > +	 * after enabling the clocks managed here.
> > +	 */
> > +	if (on) {
> > +		ret = ufshcd_vops_setup_clocks(hba, on);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  out:
> >  	if (ret) {
> >  		list_for_each_entry(clki, head, list) {
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> > Forum,
> > a Linux Foundation Collaborative Project
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" 
> > in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" 
> in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project