2019-12-11 08:50:00

by Can Guo

[permalink] [raw]
Subject: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it

In ufshcd_remove(), after SCSI host is removed, put it once so that its
resources can be released.

Signed-off-by: Can Guo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b5966fa..a86b0fd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
ufs_bsg_remove(hba);
ufs_sysfs_remove_nodes(hba->dev);
scsi_remove_host(hba->host);
+ scsi_host_put(hba->host);
/* disable interrupts */
ufshcd_disable_intr(hba, hba->intr_mask);
ufshcd_hba_stop(hba, true);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


2019-12-11 10:39:03

by Avri Altman

[permalink] [raw]
Subject: RE: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it


>
> In ufshcd_remove(), after SCSI host is removed, put it once so that its resources
> can be released.
>
> Signed-off-by: Can Guo <[email protected]>

This is not really part of this patchset, is it?

> ---
> drivers/scsi/ufs/ufshcd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> b5966fa..a86b0fd 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
> ufs_bsg_remove(hba);
> ufs_sysfs_remove_nodes(hba->dev);
> scsi_remove_host(hba->host);
> + scsi_host_put(hba->host);
> /* disable interrupts */
> ufshcd_disable_intr(hba, hba->intr_mask);
> ufshcd_hba_stop(hba, true);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

2019-12-11 11:09:03

by Can Guo

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it

On 2019-12-11 18:37, Avri Altman wrote:
>>
>> In ufshcd_remove(), after SCSI host is removed, put it once so that
>> its resources
>> can be released.
>>
>> Signed-off-by: Can Guo <[email protected]>
>
> This is not really part of this patchset, is it?
>

Hi Avri,

I put this change in the same patchset due to
#1. The main patch has dependency on it
#2. Consider a scenario where platform driver is also compiled as a
module, say ufs_qcom.ko.
In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko. If do
insmod ufs-qcom.ko
then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without this
change, because scsi
host was not release, the new scsi host will have a different host
number, meaning
hba->host->host_no will be 1, but not 0. Now if do insmod ufs-bsg.ko
now, the ufs-bsg dev
created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep
trying these operations,
the ufs-bsg device's name will be messed up. This change make sure
after ufs-qcom.ko is removed
and installed back, its hba->host->host_no stays 0, so that ufs-bsg
device name stays same.

Thanks,

Can Guo.

>> ---
>> drivers/scsi/ufs/ufshcd.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index
>> b5966fa..a86b0fd 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
>> ufs_bsg_remove(hba);
>> ufs_sysfs_remove_nodes(hba->dev);
>> scsi_remove_host(hba->host);
>> + scsi_host_put(hba->host);
>> /* disable interrupts */
>> ufshcd_disable_intr(hba, hba->intr_mask);
>> ufshcd_hba_stop(hba, true);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project

2019-12-11 11:23:14

by Avri Altman

[permalink] [raw]
Subject: RE: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it

>
>
> On 2019-12-11 18:37, Avri Altman wrote:
> >>
> >> In ufshcd_remove(), after SCSI host is removed, put it once so that
> >> its resources can be released.
> >>
> >> Signed-off-by: Can Guo <[email protected]>
> >
> > This is not really part of this patchset, is it?
> >
>
> Hi Avri,
>
> I put this change in the same patchset due to #1. The main patch has
> dependency on it #2. Consider a scenario where platform driver is also compiled
> as a module, say ufs_qcom.ko.
> In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko. If do insmod
> ufs-qcom.ko
> then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without this
> change, because scsi
> host was not release, the new scsi host will have a different host number,
> meaning
> hba->host->host_no will be 1, but not 0. Now if do insmod ufs-bsg.ko now,
> the ufs-bsg dev
> created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep trying these
> operations,
> the ufs-bsg device's name will be messed up. This change make sure after ufs-
> qcom.ko is removed
> and installed back, its hba->host->host_no stays 0, so that ufs-bsg device
> name stays same.
Looks like we needed to manage the ufs-bsg nodes using an IDA, instead of host_no?


>
> Thanks,
>
> Can Guo.
>
> >> ---
> >> drivers/scsi/ufs/ufshcd.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> >> index b5966fa..a86b0fd 100644
> >> --- a/drivers/scsi/ufs/ufshcd.c
> >> +++ b/drivers/scsi/ufs/ufshcd.c
> >> @@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
> >> ufs_bsg_remove(hba);
> >> ufs_sysfs_remove_nodes(hba->dev);
> >> scsi_remove_host(hba->host);
> >> + scsi_host_put(hba->host);
> >> /* disable interrupts */
> >> ufshcd_disable_intr(hba, hba->intr_mask);
> >> ufshcd_hba_stop(hba, true);
> >> --
> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> >> Forum, a Linux Foundation Collaborative Project

2019-12-11 11:46:10

by Can Guo

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it

On 2019-12-11 19:22, Avri Altman wrote:
>>
>>
>> On 2019-12-11 18:37, Avri Altman wrote:
>> >>
>> >> In ufshcd_remove(), after SCSI host is removed, put it once so that
>> >> its resources can be released.
>> >>
>> >> Signed-off-by: Can Guo <[email protected]>
>> >
>> > This is not really part of this patchset, is it?
>> >
>>
>> Hi Avri,
>>
>> I put this change in the same patchset due to #1. The main patch has
>> dependency on it #2. Consider a scenario where platform driver is also
>> compiled
>> as a module, say ufs_qcom.ko.
>> In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko. If
>> do insmod
>> ufs-qcom.ko
>> then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without
>> this
>> change, because scsi
>> host was not release, the new scsi host will have a different
>> host number,
>> meaning
>> hba->host->host_no will be 1, but not 0. Now if do insmod
>> ufs-bsg.ko now,
>> the ufs-bsg dev
>> created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep
>> trying these
>> operations,
>> the ufs-bsg device's name will be messed up. This change make
>> sure after ufs-
>> qcom.ko is removed
>> and installed back, its hba->host->host_no stays 0, so that
>> ufs-bsg device
>> name stays same.
> Looks like we needed to manage the ufs-bsg nodes using an IDA, instead
> of host_no?
>
>

Marking one ufs-bsg dev with host_no still has its advantage. If we have
more
than one hba host, we can find the right ufs-bsgX dev by reading the
sg/sd/bsg
device's host->host_unique_id (through SCSI_IOCTL_GET_IDLUN for
example).
But If ufs-bsg has its own ID, we will lost this "mapping".

Thanks,

Can Guo.

>>
>> Thanks,
>>
>> Can Guo.
>>
>> >> ---
>> >> drivers/scsi/ufs/ufshcd.c | 1 +
>> >> 1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> >> index b5966fa..a86b0fd 100644
>> >> --- a/drivers/scsi/ufs/ufshcd.c
>> >> +++ b/drivers/scsi/ufs/ufshcd.c
>> >> @@ -8251,6 +8251,7 @@ void ufshcd_remove(struct ufs_hba *hba)
>> >> ufs_bsg_remove(hba);
>> >> ufs_sysfs_remove_nodes(hba->dev);
>> >> scsi_remove_host(hba->host);
>> >> + scsi_host_put(hba->host);
>> >> /* disable interrupts */
>> >> ufshcd_disable_intr(hba, hba->intr_mask);
>> >> ufshcd_hba_stop(hba, true);
>> >> --
>> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> >> Forum, a Linux Foundation Collaborative Project

2019-12-11 13:45:33

by Avri Altman

[permalink] [raw]
Subject: RE: [PATCH v2 1/3] scsi: ufs: Put SCSI host after remove it


> On 2019-12-11 19:22, Avri Altman wrote:
> >>
> >>
> >> On 2019-12-11 18:37, Avri Altman wrote:
> >> >>
> >> >> In ufshcd_remove(), after SCSI host is removed, put it once so
> >> >> that its resources can be released.
> >> >>
> >> >> Signed-off-by: Can Guo <[email protected]>
> >> >
> >> > This is not really part of this patchset, is it?
> >> >
> >>
> >> Hi Avri,
> >>
> >> I put this change in the same patchset due to #1. The main patch has
> >> dependency on it #2. Consider a scenario where platform driver is
> >> also compiled as a module, say ufs_qcom.ko.
> >> In this case, we have two modules, ufs-qcom.ko and ufs-bsg.ko.
> >> If do insmod ufs-qcom.ko
> >> then rmmod ufs-qcom.ko and do insmod ufs-qcom.ko again, without
> >> this change, because scsi
> >> host was not release, the new scsi host will have a different
> >> host number, meaning
> >> hba->host->host_no will be 1, but not 0. Now if do insmod
> >> ufs-bsg.ko now, the ufs-bsg dev
> >> created in /dev/bsg/ will be ufs-bsg1, but not ufs-bsg0. If keep
> >> trying these operations,
> >> the ufs-bsg device's name will be messed up. This change make
> >> sure after ufs- qcom.ko is removed
> >> and installed back, its hba->host->host_no stays 0, so that
> >> ufs-bsg device name stays same.
> > Looks like we needed to manage the ufs-bsg nodes using an IDA, instead
> > of host_no?
> >
> >
>
> Marking one ufs-bsg dev with host_no still has its advantage. If we have more
> than one hba host, we can find the right ufs-bsgX dev by reading the sg/sd/bsg
> device's host->host_unique_id (through SCSI_IOCTL_GET_IDLUN for example).
> But If ufs-bsg has its own ID, we will lost this "mapping".
OK.