linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: qrtr: Unprepare MHI channels during remove
@ 2020-11-18 18:20 Bhaumik Bhatt
  2020-11-18 18:34 ` Jeffrey Hugo
  2020-11-20  5:10 ` Jakub Kicinski
  0 siblings, 2 replies; 10+ messages in thread
From: Bhaumik Bhatt @ 2020-11-18 18:20 UTC (permalink / raw)
  To: manivannan.sadhasivam
  Cc: kvalo, linux-wireless, cjhuang, linux-arm-msm, hemantk,
	linux-kernel, ath11k, clew, loic.poulain, netdev, Bhaumik Bhatt

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@codeaurora.org>
---
 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);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-18 18:20 [PATCH] net: qrtr: Unprepare MHI channels during remove Bhaumik Bhatt
@ 2020-11-18 18:34 ` Jeffrey Hugo
  2020-11-18 19:13   ` Bhaumik Bhatt
       [not found]   ` <CAMZdPi_b0=qFNGi1yUke3Dip2bi-zW4ULTg8W4nbyPyEsE3D4w@mail.gmail.com>
  2020-11-20  5:10 ` Jakub Kicinski
  1 sibling, 2 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2020-11-18 18:34 UTC (permalink / raw)
  To: Bhaumik Bhatt, manivannan.sadhasivam
  Cc: kvalo, linux-wireless, cjhuang, linux-arm-msm, hemantk,
	linux-kernel, ath11k, clew, loic.poulain, netdev

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@codeaurora.org>
> ---
>   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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-18 18:34 ` Jeffrey Hugo
@ 2020-11-18 19:13   ` Bhaumik Bhatt
       [not found]   ` <CAMZdPi_b0=qFNGi1yUke3Dip2bi-zW4ULTg8W4nbyPyEsE3D4w@mail.gmail.com>
  1 sibling, 0 replies; 10+ messages in thread
From: Bhaumik Bhatt @ 2020-11-18 19:13 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: manivannan.sadhasivam, kvalo, linux-wireless, cjhuang,
	linux-arm-msm, hemantk, linux-kernel, ath11k, clew, loic.poulain,
	netdev, jhugo=codeaurora.org

Hi Jeff,
On 2020-11-18 10:34 AM, Jeffrey Hugo wrote:
> 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@codeaurora.org>
>> ---
>>   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.
It allows fair and common treatment for all client drivers.
> 
> 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.
It isn't really outside of a client's control every time. If a client 
driver
module is unloaded for example, it should be in their responsibility to 
clean
up and send commands to close those channels which allows the device to 
clean
up the context.

In the event of a kernel panic or some device crash outside of a 
client's
control, this function will just free some memory and return right away 
as MHI
knows not to pursue it over the link anyway.

Some client drivers depend on USB or other drivers, which allows 
flexibility on
their end as to when to call these MHI prepare/unprepare for transfer 
APIs.

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
       [not found]   ` <CAMZdPi_b0=qFNGi1yUke3Dip2bi-zW4ULTg8W4nbyPyEsE3D4w@mail.gmail.com>
@ 2020-11-18 19:34     ` Jeffrey Hugo
  2020-11-19 19:02       ` Bhaumik Bhatt
  0 siblings, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2020-11-18 19:34 UTC (permalink / raw)
  To: Loic Poulain
  Cc: Bhaumik Bhatt, ath11k, cjhuang, clew, hemantk, kvalo,
	linux-arm-msm, linux-kernel, linux-wireless,
	manivannan.sadhasivam, netdev

On 11/18/2020 12:14 PM, Loic Poulain wrote:
> 
> 
> Le mer. 18 nov. 2020 à 19:34, Jeffrey Hugo <jhugo@codeaurora.org 
> <mailto:jhugo@codeaurora.org>> 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@codeaurora.org
>     <mailto:bbhatt@codeaurora.org>>
>      > ---
>      >   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.


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-18 19:34     ` Jeffrey Hugo
@ 2020-11-19 19:02       ` Bhaumik Bhatt
  2020-11-25 18:01         ` Jeffrey Hugo
  0 siblings, 1 reply; 10+ messages in thread
From: Bhaumik Bhatt @ 2020-11-19 19:02 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Loic Poulain, ath11k, cjhuang, clew, hemantk, kvalo,
	linux-arm-msm, linux-kernel, linux-wireless,
	manivannan.sadhasivam, netdev, jhugo=codeaurora.org

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@codeaurora.org 
>> <mailto:jhugo@codeaurora.org>> 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@codeaurora.org
>>     <mailto:bbhatt@codeaurora.org>>
>>      > ---
>>      >   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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-18 18:20 [PATCH] net: qrtr: Unprepare MHI channels during remove Bhaumik Bhatt
  2020-11-18 18:34 ` Jeffrey Hugo
@ 2020-11-20  5:10 ` Jakub Kicinski
  2020-11-20  6:15   ` Manivannan Sadhasivam
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-11-20  5:10 UTC (permalink / raw)
  To: Bhaumik Bhatt, manivannan.sadhasivam
  Cc: kvalo, linux-wireless, cjhuang, linux-arm-msm, hemantk,
	linux-kernel, ath11k, clew, loic.poulain, netdev

On Wed, 18 Nov 2020 10:20:25 -0800 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@codeaurora.org>

Patch seems reasonable, Mani are you taking it or should I?

Bhaumik would you mind adding a Fixes tag to be clear where 
the issue was introduced?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-20  5:10 ` Jakub Kicinski
@ 2020-11-20  6:15   ` Manivannan Sadhasivam
  2020-11-20  6:18     ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Manivannan Sadhasivam @ 2020-11-20  6:15 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Bhaumik Bhatt, kvalo, linux-wireless, cjhuang, linux-arm-msm,
	hemantk, linux-kernel, ath11k, clew, loic.poulain, netdev

On Thu, Nov 19, 2020 at 09:10:46PM -0800, Jakub Kicinski wrote:
> On Wed, 18 Nov 2020 10:20:25 -0800 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@codeaurora.org>
> 
> Patch seems reasonable, Mani are you taking it or should I?
> 

Since I already picked up one qrtr patch, makese sense to pick this
also.

> Bhaumik would you mind adding a Fixes tag to be clear where 
> the issue was introduced?

This is due to the MHI auto-start change which just got queued into
mhi-next. I don't think we need a Fixes tag.

Jakub, can you please provide your ack so that I can take it?

Thanks,
Mani

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-20  6:15   ` Manivannan Sadhasivam
@ 2020-11-20  6:18     ` Jakub Kicinski
  2020-11-20  6:23       ` Manivannan Sadhasivam
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2020-11-20  6:18 UTC (permalink / raw)
  To: Manivannan Sadhasivam
  Cc: Bhaumik Bhatt, kvalo, linux-wireless, cjhuang, linux-arm-msm,
	hemantk, linux-kernel, ath11k, clew, loic.poulain, netdev

On Fri, 20 Nov 2020 11:45:12 +0530 Manivannan Sadhasivam wrote:
> Jakub, can you please provide your ack so that I can take it?

Sure:

Acked-by: Jakub Kicinski <kuba@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-20  6:18     ` Jakub Kicinski
@ 2020-11-20  6:23       ` Manivannan Sadhasivam
  0 siblings, 0 replies; 10+ messages in thread
From: Manivannan Sadhasivam @ 2020-11-20  6:23 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Bhaumik Bhatt, kvalo, linux-wireless, cjhuang, linux-arm-msm,
	hemantk, linux-kernel, ath11k, clew, loic.poulain, netdev

On Thu, Nov 19, 2020 at 10:18:28PM -0800, Jakub Kicinski wrote:
> On Fri, 20 Nov 2020 11:45:12 +0530 Manivannan Sadhasivam wrote:
> > Jakub, can you please provide your ack so that I can take it?
> 
> Sure:
> 
> Acked-by: Jakub Kicinski <kuba@kernel.org>

Patch applied to mhi-ath11k-immutable.

Thanks,
Mani

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] net: qrtr: Unprepare MHI channels during remove
  2020-11-19 19:02       ` Bhaumik Bhatt
@ 2020-11-25 18:01         ` Jeffrey Hugo
  0 siblings, 0 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2020-11-25 18:01 UTC (permalink / raw)
  To: bbhatt
  Cc: Loic Poulain, ath11k, cjhuang, clew, hemantk, kvalo,
	linux-arm-msm, linux-kernel, linux-wireless,
	manivannan.sadhasivam, netdev, jhugo=codeaurora.org

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@codeaurora.org 
>>> <mailto:jhugo@codeaurora.org>> 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@codeaurora.org
>>>     <mailto:bbhatt@codeaurora.org>>
>>>      > ---
>>>      >   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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-11-25 18:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 18:20 [PATCH] net: qrtr: Unprepare MHI channels during remove Bhaumik Bhatt
2020-11-18 18:34 ` Jeffrey Hugo
2020-11-18 19:13   ` Bhaumik Bhatt
     [not found]   ` <CAMZdPi_b0=qFNGi1yUke3Dip2bi-zW4ULTg8W4nbyPyEsE3D4w@mail.gmail.com>
2020-11-18 19:34     ` Jeffrey Hugo
2020-11-19 19:02       ` Bhaumik Bhatt
2020-11-25 18:01         ` Jeffrey Hugo
2020-11-20  5:10 ` Jakub Kicinski
2020-11-20  6:15   ` Manivannan Sadhasivam
2020-11-20  6:18     ` Jakub Kicinski
2020-11-20  6:23       ` Manivannan Sadhasivam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).