Re: [PATCH 3/3] bus: mhi: core: Remove link_status() callback

[Jeffrey Hugo <jhugo@codeaurora.org>]

On 4/6/2020 11:58 PM, Manivannan Sadhasivam wrote:
> Hi Jeff,
> 
> On Mon, Apr 06, 2020 at 03:04:37PM -0600, Jeffrey Hugo wrote:
>> If the MHI core detects invalid data due to a PCI read, it calls into
>> the controller via link_status() to double check that the link is infact
>> down.  All in all, this is pretty pointless, and racy.  There are no good
>> reasons for this, and only drawbacks.
>>
>> Its pointless because chances are, the controller is going to do the same
>> thing to determine if the link is down - attempt a PCI access and compare
>> the result.  This does not make the link status decision any smarter.
>>
>> Its racy because its possible that the link was down at the time of the
>> MHI core access, but then recovered before the controller access.  In this
>> case, the controller will indicate the link is not down, and the MHI core
>> will precede to use a bad value as the MHI core does not attempt to retry
>> the access.
>>
>> Retrying the access in the MHI core is a bad idea because again, it is
>> racy - what if the link is down again?  Furthermore, there may be some
>> higher level state associated with the link status, that is now invalid
>> because the link went down.
>>
>> The only reason why the MHI core could see "invalid" data when doing a PCI
>> access, that is actually valid, is if the register actually contained the
>> PCI spec defined sentinel for an invalid access.  In this case, it is
>> arguable that the MHI implementation broken, and should be fixed, not
>> worked around.
>>
>> Therefore, remove the link_status() callback before anyone attempts to
>> implement it.
>>
>> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> 
> LGTM. But as per the IRC discussion I'd like the mhi_reg_read() to be
> implemented as a callback in mhi_controller struct inorder to truly make MHI
> a PCI agnostic bus.
> 
> Since we don't have any controller driver in mainline, I think it is the
> good time to do this change.

No problem.  I thought you might prefer that approach, hence the 
discussion.  :)

Do you want that included in this change, or as a follow up?

> 
> For this,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Thanks,
> Mani
> 
>> ---
>>   drivers/bus/mhi/core/init.c | 6 ++----
>>   drivers/bus/mhi/core/main.c | 5 ++---
>>   include/linux/mhi.h         | 2 --
>>   3 files changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index b38359c..2af08d57 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -812,10 +812,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>>   	if (!mhi_cntrl)
>>   		return -EINVAL;
>>   
>> -	if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put)
>> -		return -EINVAL;
>> -
>> -	if (!mhi_cntrl->status_cb || !mhi_cntrl->link_status)
>> +	if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put ||
>> +	    !mhi_cntrl->status_cb)
>>   		return -EINVAL;
>>   
>>   	ret = parse_config(mhi_cntrl, config);
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index eb4256b..473278b8 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -20,9 +20,8 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
>>   {
>>   	u32 tmp = readl(base + offset);
>>   
>> -	/* If there is any unexpected value, query the link status */
>> -	if (PCI_INVALID_READ(tmp) &&
>> -	    mhi_cntrl->link_status(mhi_cntrl))
>> +	/* If the value is invalid, the link is down */
>> +	if (PCI_INVALID_READ(tmp))
>>   		return -EIO;
>>   
>>   	*out = tmp;
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index ad19960..be704a4 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -335,7 +335,6 @@ struct mhi_controller_config {
>>    * @syserr_worker: System error worker
>>    * @state_event: State change event
>>    * @status_cb: CB function to notify power states of the device (required)
>> - * @link_status: CB function to query link status of the device (required)
>>    * @wake_get: CB function to assert device wake (optional)
>>    * @wake_put: CB function to de-assert device wake (optional)
>>    * @wake_toggle: CB function to assert and de-assert device wake (optional)
>> @@ -417,7 +416,6 @@ struct mhi_controller {
>>   
>>   	void (*status_cb)(struct mhi_controller *mhi_cntrl,
>>   			  enum mhi_callback cb);
>> -	int (*link_status)(struct mhi_controller *mhi_cntrl);
>>   	void (*wake_get)(struct mhi_controller *mhi_cntrl, bool override);
>>   	void (*wake_put)(struct mhi_controller *mhi_cntrl, bool override);
>>   	void (*wake_toggle)(struct mhi_controller *mhi_cntrl);
>> -- 
>> Qualcomm Technologies, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.