Re: [PATCH v6 3/7] scsi: ufs: introduce common delay function

From: Bart Van Assche <bvanassche@acm.org>
To: Stanley Chu <stanley.chu@mediatek.com>
Cc: linux-scsi@vger.kernel.org,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	andy.teng@mediatek.com, jejb@linux.ibm.com,
	chun-hung.wu@mediatek.com, kuohong.wang@mediatek.com,
	linux-kernel@vger.kernel.org, avri.altman@wdc.com,
	cang@codeaurora.org, linux-mediatek@lists.infradead.org,
	peter.wang@mediatek.com, alim.akhtar@samsung.com,
	matthias.bgg@gmail.com, asutoshd@codeaurora.org,
	linux-arm-kernel@lists.infradead.org, beanhuo@micron.com
Subject: Re: [PATCH v6 3/7] scsi: ufs: introduce common delay function
Date: Mon, 16 Mar 2020 20:59:34 -0700	[thread overview]
Message-ID: <b7a6045e-9615-0cd2-9812-2871bf9ba44c@acm.org> (raw)
In-Reply-To: <1584404000.14250.28.camel@mtksdccf07>

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel