On 2020-03-16 17:13, Stanley Chu wrote:
> On Mon, 2020-03-16 at 09:23 -0700, Bart Van Assche wrote:
>> On 3/16/20 1:52 AM, Stanley Chu wrote:
>>> +void ufshcd_wait_us(unsigned long us, unsigned long tolerance, bool can_sleep)
>>> +{
>>> + if (!us)
>>> + return;
>>> +
>>> + if (us < 10 || !can_sleep)
>>> + udelay(us);
>>> + else
>>> + usleep_range(us, us + tolerance);
>>> +}
>>> +EXPORT_SYMBOL_GPL(ufshcd_wait_us);
>>
>> I don't like this function because I think it makes the UFS code harder
>> to read instead of easier. The 'can_sleep' argument is only set by one
>> caller which I think is a strong argument to remove that argument again
>> and to move the code that depends on that argument from the above
>> function into the caller. Additionally, it is not possible to comprehend
>> what a ufshcd_wait_us() call does without looking up the function
>> definition to see what the meaning of the third argument is.
>>
>> Please drop this patch.
>
> Thanks for your review and comments.
>
> If the problem is the third argument 'can_sleep' which makes the code
> not be easily comprehensible, how about just removing 'can_sleep' from
> this function and keeping left parts because this function provides good
> flexibility to users to choose udelay or usleep_range according to the
> 'us' argument?
Hi Stanley,
I think that we need to get rid of 'can_sleep' across the entire UFS
driver. As far as I can see the only context from which 'can_sleep' is
set to true is ufshcd_host_reset_and_restore() and 'can_sleep' is set to
true because ufshcd_hba_stop() is called with a spinlock held. Do you
agree that it is wrong to call udelay() while holding a spinlock() and
that doing so has a bad impact on the energy consumption of the UFS
driver? Has it already been considered to use another mechanism to
serialize REG_CONTROLLER_ENABLE changes, e.g. a mutex?
Thanks,
Bart.
