diff mbox series

[1/8] scsi: ufshpb: Cache HPB Control mode on init

Message ID 20210127151217.24760-2-avri.altman@wdc.com
State New
Headers show
Series Add Host control mode to HPB | expand

Commit Message

Avri Altman Jan. 27, 2021, 3:12 p.m. UTC
We will use it later, when we'll need to differentiate between device
and host control modes.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufshpb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Greg KH Jan. 27, 2021, 3:19 p.m. UTC | #1
On Wed, Jan 27, 2021 at 05:12:10PM +0200, Avri Altman wrote:
> We will use it later, when we'll need to differentiate between device
> and host control modes.
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>
> ---
>  drivers/scsi/ufs/ufshpb.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index d3e6c5b32328..183bdf35f2d0 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -26,6 +26,8 @@ static int tot_active_srgn_pages;
>  
>  static struct workqueue_struct *ufshpb_wq;
>  
> +static enum UFSHPB_MODE ufshpb_mode;

How are you allowed to have a single variable for a device-specific
thing?  What happens when you have two controllers or disks or whatever
you are binding to here?  How does this work at all?

This should be per-device, right?

thanks,

greg k-h
Avri Altman Jan. 31, 2021, 7:08 a.m. UTC | #2
> >

> > +static enum UFSHPB_MODE ufshpb_mode;

> 

> How are you allowed to have a single variable for a device-specific

> thing?  What happens when you have two controllers or disks or whatever

> you are binding to here?  How does this work at all?

> 

> This should be per-device, right?

Right. Done.

Not being bickering,  AFAIK, there aren't, nor will be in the foreseen future, any multi-ufs-hosts designs.
There were some talks in the past about ufs cards, but this is officially off the table.

Thanks,
Avri
Greg KH Jan. 31, 2021, 7:13 a.m. UTC | #3
On Sun, Jan 31, 2021 at 07:08:00AM +0000, Avri Altman wrote:
> > >

> > > +static enum UFSHPB_MODE ufshpb_mode;

> > 

> > How are you allowed to have a single variable for a device-specific

> > thing?  What happens when you have two controllers or disks or whatever

> > you are binding to here?  How does this work at all?

> > 

> > This should be per-device, right?

> Right. Done.

> 

> Not being bickering,  AFAIK, there aren't, nor will be in the foreseen future, any multi-ufs-hosts designs.


Why not?  What prevents someone from putting 2 PCI ufs host controllers
in a system tomorrow?

> There were some talks in the past about ufs cards, but this is officially off the table.


Never say never :)

Seriously, how can you somehow ensure that a random manufacturer will
not do this?

thanks,

greg k-h
Avri Altman Jan. 31, 2021, 7:17 a.m. UTC | #4
> On Sun, Jan 31, 2021 at 07:08:00AM +0000, Avri Altman wrote:

> > > >

> > > > +static enum UFSHPB_MODE ufshpb_mode;

> > >

> > > How are you allowed to have a single variable for a device-specific

> > > thing?  What happens when you have two controllers or disks or whatever

> > > you are binding to here?  How does this work at all?

> > >

> > > This should be per-device, right?

> > Right. Done.

> >

> > Not being bickering,  AFAIK, there aren't, nor will be in the foreseen future,

> any multi-ufs-hosts designs.

> 

> Why not?  What prevents someone from putting 2 PCI ufs host controllers

> in a system tomorrow?

> 

> > There were some talks in the past about ufs cards, but this is officially off

> the table.

> 

> Never say never :)

> 

> Seriously, how can you somehow ensure that a random manufacturer will

> not do this?

Better let the platform vendors answer this.

As for your comment - you are obviously right - I will fix this.

Thanks,
Avri
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index d3e6c5b32328..183bdf35f2d0 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -26,6 +26,8 @@  static int tot_active_srgn_pages;
 
 static struct workqueue_struct *ufshpb_wq;
 
+static enum UFSHPB_MODE ufshpb_mode;
+
 bool ufshpb_is_allowed(struct ufs_hba *hba)
 {
 	return !(hba->ufshpb_dev.hpb_disabled);
@@ -1690,10 +1692,9 @@  void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf)
 {
 	struct ufshpb_dev_info *hpb_dev_info = &hba->ufshpb_dev;
 	int version;
-	u8 hpb_mode;
 
-	hpb_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
-	if (hpb_mode == HPB_HOST_CONTROL) {
+	ufshpb_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
+	if (ufshpb_mode == HPB_HOST_CONTROL) {
 		dev_err(hba->dev, "%s: host control mode is not supported.\n",
 			__func__);
 		hpb_dev_info->hpb_disabled = true;