On 11/19/2020 12:02 PM, Bhaumik Bhatt wrote:
On 2020-11-18 11:34 AM, Jeffrey Hugo wrote:
On 11/18/2020 12:14 PM, Loic Poulain wrote:
Le mer. 18 nov. 2020 à 19:34, Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx
<mailto:jhugo@xxxxxxxxxxxxxx>> a écrit :
On 11/18/2020 11:20 AM, Bhaumik Bhatt wrote:
> Reset MHI device channels when driver remove is called due to
> module unload or any crash scenario. This will make sure that
> MHI channels no longer remain enabled for transfers since the
> MHI stack does not take care of this anymore after the auto-start
> channels feature was removed.
>
> Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx
<mailto:bbhatt@xxxxxxxxxxxxxx>>
> ---
> net/qrtr/mhi.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c
> index 7100f0b..2bf2b19 100644
> --- a/net/qrtr/mhi.c
> +++ b/net/qrtr/mhi.c
> @@ -104,6 +104,7 @@ static void qcom_mhi_qrtr_remove(struct
mhi_device *mhi_dev)
> struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
>
> qrtr_endpoint_unregister(&qdev->ep);
> + mhi_unprepare_from_transfer(mhi_dev);
> dev_set_drvdata(&mhi_dev->dev, NULL);
> }
>
>
I admit, I didn't pay much attention to the auto-start being
removed,
but this seems odd to me.
As a client, the MHI device is being removed, likely because of some
factor outside of my control, but I still need to clean it up? This
really feels like something MHI should be handling.
I think this is just about balancing operations, what is done in
probe should be undone in remove, so here channels are started in
probe and stopped/reset in remove.
I understand that perspective, but that doesn't quite match what is
going on here. Regardless of if the channel was started (prepared) in
probe, it now needs to be stopped in remove. That not balanced in all
cases
Lets assume, in response to probe(), my client driver goes and creates
some other object, maybe a socket. In response to that socket being
opened/activated by the client of my driver, I go and start the mhi
channel. Now, normally, when the socket is closed/deactivated, I stop
the MHI channel. In this case, stopping the MHI channel in remove()
is unbalanced with respect to probe(), but is now a requirement.
Now you may argue, I should close the object in response to remove,
which will then trigger the stop on the channel. That doesn't apply
to everything. For example, you cannot close an open file in the
kernel. You need to wait for userspace to close it. By the time that
happens, the mhi_dev is long gone I expect.
So if, somehow, the client driver is the one causing the remove to
occur, then yes it should probably be the one doing the stop, but
that's a narrow set of conditions, and I think having that requirement
for all scenarios is limiting.
It should be the client's responsibility to perform a clean-up though.
We cannot assume that the remove() call was due to factors outside of the
client's control at all times. You may not know if the remove() was due to
device actually crashing or just an unbind/module unload. So, it would be
better if you call it as the device should ideally not be left with a stale
channel context. >
We had an issue where a client was issuing a driver unbind without
unpreparing
the MHI channels and without Loic's patch [1], we would not issue a channel
RESET to the device resulting in incoming data to the host on those
channels
after host clean-up and an unmapped memory access and kernel panic.
So the client drivers have to do the right thing, otherwise the kernel
could crash? Sounds like you are choosing to not do defensive coding in
MHI and making your problems the client's problems.
Before releasing the resources, why haven't you issued a MHI_RESET of
the state machine, and ensured the device has ack'd the reset?
If MHI dev will be gone that NULL/status check must be present in
something that
userspace could potentially use.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/bus/mhi?h=next-20201119&id=a7f422f2f89e7d48aa66e6488444a4c7f01269d5
Thanks,
Bhaumik
---
The Qualcomm Innovation Center, 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.
