Re: [PATCH 2/8] mhi: pci-generic: Perform hard reset on remove

[Bhaumik Bhatt <bbhatt@codeaurora.org>]

On 2020-11-27 08:21 AM, Loic Poulain wrote:
> Hi Jeffrey,
> 
> On Wed, 25 Nov 2020 at 18:41, Jeffrey Hugo <jhugo@codeaurora.org> 
> wrote:
>> >>> @@ -329,6 +336,10 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>> >>>        mhi_power_down(mhi_cntrl, true);
>> >>>        mhi_unprepare_after_power_down(mhi_cntrl);
>> >>>        mhi_unregister_controller(mhi_cntrl);
>> >>> +
>> >>> +     /* MHI-layer reset could not be enough, always hard-reset the device */
>> >>> +     mhi_pci_reset(mhi_cntrl);
>> >>
>> >> Referring to MHI spec:
>> >> Hosts writes this register to trigger a reset. This can be used when the
>> >> host detects a timeout in the MHI protocol and can use the reset as a
>> >> last resort to recover the device. Host should first attempt an MHI
>> >> Reset procedure before resetting the entire device.
>> >>
>> >> What issue are you facing which requires you to do full device reset ?
>> >> Based on the spec recommendation, looks like this should be a last resort.
>> >
>> > On module unload/reload, the device does not go through cold reset and
>> > can be in error state on further reload, causing mhi power up to fail.
>> > This patch simply resets the device in remove so that it is in the
>> > exact same condition as before probing (not only MHI layer, but all
>> > the device context), I think it makes sense and prevents any
>> > unexpected state on further reloading. Note also that unloading the
>> > module (or unbinding the device) is not something that usually
>> > happens, except when the user does it explicitly for any reason.
>> 
>> This seems unnecessary to me.  Qaic has the same usecase, and the MHI
>> state machine reset is sufficient.  Perhaps you have a unique edge 
>> case
>> though.
>> 
>> However, you are implementing the soc_reset functionality in your
>> driver, when its a common MHI functionality, and is something I would
>> like to use.  If you dig back, I proposed a sysfs extension to expose
>> that to userspace, but I have a desire to use it from my driver, same 
>> as
>> you.
>> 
>> Would you please make MHI core changes to expose the soc_reset
>> functionality out so that multiple drivers can use a common 
>> implementation?
> 
> I overlooked this reply, going to move that into MHI core, as you 
> suggested.
> 
> Thanks,
> Loic
Yes, this makes sense to do as Jeff suggested.
Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project