Re: [PATCH 05/25] scsi: hisi_sas: allocate memories and create pools

Re: [PATCH 05/25] scsi: hisi_sas: allocate memories and create pools

[Arnd Bergmann]

On Monday 12 October 2015 23:20:17 John Garry wrote:
> +       interrupt_count = of_property_count_u32_elems(np, "interrupts");
> +       if (interrupt_count < 0)
> +               goto err_out;
> +
> +       if (of_property_read_u32(np, "#interrupt-cells", &interrupt_cells))
> +               goto err_out;
> +
> +       hisi_hba->int_names = devm_kcalloc(&pdev->dev,
> +                                          interrupt_count / interrupt_cells,
> +                                          HISI_SAS_NAME_LEN,
> +                                          GFP_KERNEL);
> 

This computation looks wrong: the "interrupts" property refers to interrupts
that are referenced by this node and provided by an interrupt-controller,
while the "#interrupt-cells" property refers to interrupts provided by
this node. They don't need to have any relation.

	Arnd

Re: [PATCH 04/25] scsi: hisi_sas: add scsi host registration

[Arnd Bergmann]

On Monday 12 October 2015 23:20:16 John Garry wrote:
> +
> +       shost = scsi_host_alloc(&hisi_sas_sht, sizeof(void *));
> +       if (!shost)
> +               return -ENOMEM;
> +
> +       hisi_hba = hisi_sas_hba_alloc(pdev, shost, np);
> +       if (!hisi_hba) {
> +               rc = -ENOMEM;
> +               goto err_out_ha;
> +       }

You can collapse the allocations into one and just pass sizeof(*hisi_hba)
instead of sizeof(void *) above.


> +       sha = SHOST_TO_SAS_HA(shost) = &hisi_hba->sha;
> +       platform_set_drvdata(pdev, sha);
> +
> +       phy_nr = port_nr = HISI_SAS_MAX_PHYS;
> +
> +       arr_phy = devm_kcalloc(dev, phy_nr, sizeof(void *), GFP_KERNEL);
> +       arr_port = devm_kcalloc(dev, port_nr, sizeof(void *), GFP_KERNEL);
> +       if (!arr_phy || !arr_port)
> +               return -ENOMEM;

And since these are fixed-size arrays, they can be moved in there as well.

	Arnd

Re: [PATCH 07/25] scsi: hisi_sas: add ioremap for device HW

[Arnd Bergmann]

On Monday 12 October 2015 23:20:19 John Garry wrote:
> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba)
> +{
> +       struct platform_device *pdev = hisi_hba->pdev;
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       hisi_hba->regs = devm_ioremap(dev,
> +                                     res->start,
> +                                     resource_size(res));
> +       if (!hisi_hba->regs)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       hisi_hba->ctrl_regs = devm_ioremap(dev,
> +                                          res->start,
> +                                          resource_size(res));
> +       if (!hisi_hba->ctrl_regs)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
>  
>  static const struct of_device_id sas_of_match[] = {
> 

Better use devm_ioremap_resource() here, which registers the resource so they
are checked for conflicts and listed in /proc/iomem.

	Arnd

Re: [PATCH 12/25] scsi: hisi_sas: add v1 HW initialisation code

[Arnd Bergmann]

On Monday 12 October 2015 23:20:24 John Garry wrote:
> @@ -387,6 +392,21 @@ static int hisi_sas_probe(struct platform_device *pdev)
>         }
>  
>         hisi_sas_init_add(hisi_hba);
> +
> +       rc = hw_init_v1_hw(hisi_hba);
> +       if (rc)
> +               goto err_out_ha;
> +
> +       rc = interrupt_init_v1_hw(hisi_hba);
> +       if (rc)
> +               goto err_out_ha;
> +
> +       rc = interrupt_openall_v1_hw(hisi_hba);
> +       if (rc)
> +               goto err_out_ha;
> +
> +       phys_init_v1_hw(hisi_hba);
> +
>         rc = scsi_add_host(shost, &pdev->dev);
>         if (rc)
>                 goto err_out_ha;
> 

As the probe function matches against the "hisilicon,sas-controller-v1"
compatible string and contains mostly code that is specific to v1, I
think it would be cleaner to move that to the hisi_sas_v1_hw.c as well
and make the functions above static but make the common functions
called here (hisi_sas_init_add etc) global.

That would also include the hisi_sas_driver structure.

	Arnd

Re: [PATCH 04/25] scsi: hisi_sas: add scsi host registration

[John Garry]

On 12/10/2015 16:21, Arnd Bergmann wrote:
> On Monday 12 October 2015 23:20:16 John Garry wrote:
>> +
>> +       shost = scsi_host_alloc(&hisi_sas_sht, sizeof(void *));
>> +       if (!shost)
>> +               return -ENOMEM;
>> +
>> +       hisi_hba = hisi_sas_hba_alloc(pdev, shost, np);
>> +       if (!hisi_hba) {
>> +               rc = -ENOMEM;
>> +               goto err_out_ha;
>> +       }
>
> You can collapse the allocations into one and just pass sizeof(*hisi_hba)
> instead of sizeof(void *) above.
>
Will address.

>
>> +       sha = SHOST_TO_SAS_HA(shost) = &hisi_hba->sha;
>> +       platform_set_drvdata(pdev, sha);
>> +
>> +       phy_nr = port_nr = HISI_SAS_MAX_PHYS;
>> +
>> +       arr_phy = devm_kcalloc(dev, phy_nr, sizeof(void *), GFP_KERNEL);
>> +       arr_port = devm_kcalloc(dev, port_nr, sizeof(void *), GFP_KERNEL);
>> +       if (!arr_phy || !arr_port)
>> +               return -ENOMEM;
>
> And since these are fixed-size arrays, they can be moved in there as well.
>
In a later patch we set as follows:
phy_nr = port_nr = hisi_hba->n_phy;

You did say in our earlier private review that we could add statically 
to hba strcut, but I commented that other vendors do similar so I would 
wait for more input.

> 	Arnd
>
> .
>

Thanks,
John

Re: [PATCH 05/25] scsi: hisi_sas: allocate memories and create pools

[zhangfei]

Hi, Arnd

On 10/12/2015 11:15 PM, Arnd Bergmann wrote:
> On Monday 12 October 2015 23:20:17 John Garry wrote:
>> +       interrupt_count = of_property_count_u32_elems(np, "interrupts");
>> +       if (interrupt_count < 0)
>> +               goto err_out;
>> +
>> +       if (of_property_read_u32(np, "#interrupt-cells", &interrupt_cells))
>> +               goto err_out;
>> +
>> +       hisi_hba->int_names = devm_kcalloc(&pdev->dev,
>> +                                          interrupt_count / interrupt_cells,
>> +                                          HISI_SAS_NAME_LEN,
>> +                                          GFP_KERNEL);
>>
>
> This computation looks wrong: the "interrupts" property refers to interrupts
> that are referenced by this node and provided by an interrupt-controller,
> while the "#interrupt-cells" property refers to interrupts provided by
> this node. They don't need to have any relation.
>
We will use of_irq_count instead.

Thanks

Re: [PATCH 07/25] scsi: hisi_sas: add ioremap for device HW

[zhangfei]

On 10/12/2015 11:21 PM, Arnd Bergmann wrote:
> On Monday 12 October 2015 23:20:19 John Garry wrote:
>> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba)
>> +{
>> +       struct platform_device *pdev = hisi_hba->pdev;
>> +       struct device *dev = &pdev->dev;
>> +       struct resource *res;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       hisi_hba->regs = devm_ioremap(dev,
>> +                                     res->start,
>> +                                     resource_size(res));
>> +       if (!hisi_hba->regs)
>> +               return -ENOMEM;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +       hisi_hba->ctrl_regs = devm_ioremap(dev,
>> +                                          res->start,
>> +                                          resource_size(res));
>> +       if (!hisi_hba->ctrl_regs)
>> +               return -ENOMEM;
>> +
>> +       return 0;
>> +}
>>
>>   static const struct of_device_id sas_of_match[] = {
>>
>
> Better use devm_ioremap_resource() here, which registers the resource so they
> are checked for conflicts and listed in /proc/iomem.
>

Yes, hisi_hba->regs can use devm_ioremap_resource.

However ctrl_regs have to use devm_ioremap, since the address are 
sharing among different nodes, unfortunately, and devm_ioremap_resource 
will fail.

Thanks

Re: [PATCH 04/25] scsi: hisi_sas: add scsi host registration

[Arnd Bergmann]

On Tuesday 13 October 2015 10:16:55 John Garry wrote:
> 
> >
> >> +       sha = SHOST_TO_SAS_HA(shost) = &hisi_hba->sha;
> >> +       platform_set_drvdata(pdev, sha);
> >> +
> >> +       phy_nr = port_nr = HISI_SAS_MAX_PHYS;
> >> +
> >> +       arr_phy = devm_kcalloc(dev, phy_nr, sizeof(void *), GFP_KERNEL);
> >> +       arr_port = devm_kcalloc(dev, port_nr, sizeof(void *), GFP_KERNEL);
> >> +       if (!arr_phy || !arr_port)
> >> +               return -ENOMEM;
> >
> > And since these are fixed-size arrays, they can be moved in there as well.
> >
> In a later patch we set as follows:
> phy_nr = port_nr = hisi_hba->n_phy;
> 
> You did say in our earlier private review that we could add statically 
> to hba strcut, but I commented that other vendors do similar so I would 
> wait for more input.

Ok, I forgot about that.

	Arnd

Re: [PATCH 07/25] scsi: hisi_sas: add ioremap for device HW

[Arnd Bergmann]

On Tuesday 13 October 2015 17:47:02 zhangfei wrote:
> On 10/12/2015 11:21 PM, Arnd Bergmann wrote:
> > On Monday 12 October 2015 23:20:19 John Garry wrote:
> >> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba)
> >> +{
> >> +       struct platform_device *pdev = hisi_hba->pdev;
> >> +       struct device *dev = &pdev->dev;
> >> +       struct resource *res;
> >> +
> >> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +       hisi_hba->regs = devm_ioremap(dev,
> >> +                                     res->start,
> >> +                                     resource_size(res));
> >> +       if (!hisi_hba->regs)
> >> +               return -ENOMEM;
> >> +
> >> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >> +       hisi_hba->ctrl_regs = devm_ioremap(dev,
> >> +                                          res->start,
> >> +                                          resource_size(res));
> >> +       if (!hisi_hba->ctrl_regs)
> >> +               return -ENOMEM;
> >> +
> >> +       return 0;
> >> +}
> >>
> >>   static const struct of_device_id sas_of_match[] = {
> >>
> >
> > Better use devm_ioremap_resource() here, which registers the resource so they
> > are checked for conflicts and listed in /proc/iomem.
> >
> 
> Yes, hisi_hba->regs can use devm_ioremap_resource.
> 
> However ctrl_regs have to use devm_ioremap, since the address are 
> sharing among different nodes, unfortunately, and devm_ioremap_resource 
> will fail.

This sounds like it should be fixed in the DT binding then, to ensure
that the ranges don't overlap.

Mapping the same register region multiple times is generally considered
a bad idea because the drivers that map them often don't have global
locks that serialize the access, so it's better to have code in place
that ensures that they are distinct.

What is the purpose of the ctrl_regs region, and why is it shared
across multiple devices?

Are all users of these registers in the same driver?

	Arnd

Re: [PATCH 12/25] scsi: hisi_sas: add v1 HW initialisation code

[John Garry]

On 12/10/2015 19:46, Arnd Bergmann wrote:
> On Monday 12 October 2015 23:20:24 John Garry wrote:
>> @@ -387,6 +392,21 @@ static int hisi_sas_probe(struct platform_device *pdev)
>>          }
>>
>>          hisi_sas_init_add(hisi_hba);
>> +
>> +       rc = hw_init_v1_hw(hisi_hba);
>> +       if (rc)
>> +               goto err_out_ha;
>> +
>> +       rc = interrupt_init_v1_hw(hisi_hba);
>> +       if (rc)
>> +               goto err_out_ha;
>> +
>> +       rc = interrupt_openall_v1_hw(hisi_hba);
>> +       if (rc)
>> +               goto err_out_ha;
>> +
>> +       phys_init_v1_hw(hisi_hba);
>> +
>>          rc = scsi_add_host(shost, &pdev->dev);
>>          if (rc)
>>                  goto err_out_ha;
>>
>
> As the probe function matches against the "hisilicon,sas-controller-v1"
> compatible string and contains mostly code that is specific to v1, I
> think it would be cleaner to move that to the hisi_sas_v1_hw.c as well
> and make the functions above static but make the common functions
> called here (hisi_sas_init_add etc) global.
>
> That would also include the hisi_sas_driver structure.
>
> 	Arnd
>
> .
>
Hello,

Just to be clear, are you saying that you would prefer hisi_sas_probe() 
and struct hisi_sas_driver to be relocated to hisi_sas_v1_hw.c?

I wanted to keep hisi_sas_v1_hw.c containing only code which accesses HW.

I could consoldate the calls of hw_init_v1_hw(), interrupt_init_v1_hw(), 
interrupt_openall_v1_hw(), and phys_init_v1_hw() to a single function hw 
init function call. Then most of the code in the probe function will not 
be specific to v1.

Regards,
John

Re: [PATCH 12/25] scsi: hisi_sas: add v1 HW initialisation code

[Arnd Bergmann]

On Tuesday 13 October 2015 13:44:26 John Garry wrote:
> Hello,
> 
> Just to be clear, are you saying that you would prefer hisi_sas_probe() 
> and struct hisi_sas_driver to be relocated to hisi_sas_v1_hw.c?

Correct.

> I wanted to keep hisi_sas_v1_hw.c containing only code which accesses HW.
> 
> I could consoldate the calls of hw_init_v1_hw(), interrupt_init_v1_hw(), 
> interrupt_openall_v1_hw(), and phys_init_v1_hw() to a single function hw 
> init function call. Then most of the code in the probe function will not 
> be specific to v1.

This is more about correct layering of the code. Generally we want the
more specific modules on top to call into the more general code in
the bottom of the stack. Shared code should know as little as possible
about the specific implementations it is shared with.

	Arnd

Re: [PATCH 07/25] scsi: hisi_sas: add ioremap for device HW

[zhangfei]

On 10/13/2015 08:20 PM, Arnd Bergmann wrote:
> On Tuesday 13 October 2015 17:47:02 zhangfei wrote:
>> On 10/12/2015 11:21 PM, Arnd Bergmann wrote:
>>> On Monday 12 October 2015 23:20:19 John Garry wrote:
>>>> +int hisi_sas_ioremap(struct hisi_hba *hisi_hba)
>>>> +{
>>>> +       struct platform_device *pdev = hisi_hba->pdev;
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct resource *res;
>>>> +
>>>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +       hisi_hba->regs = devm_ioremap(dev,
>>>> +                                     res->start,
>>>> +                                     resource_size(res));
>>>> +       if (!hisi_hba->regs)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +       hisi_hba->ctrl_regs = devm_ioremap(dev,
>>>> +                                          res->start,
>>>> +                                          resource_size(res));
>>>> +       if (!hisi_hba->ctrl_regs)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       return 0;
>>>> +}
>>>>
>>>>    static const struct of_device_id sas_of_match[] = {
>>>>
>>>
>>> Better use devm_ioremap_resource() here, which registers the resource so they
>>> are checked for conflicts and listed in /proc/iomem.
>>>
>>
>> Yes, hisi_hba->regs can use devm_ioremap_resource.
>>
>> However ctrl_regs have to use devm_ioremap, since the address are
>> sharing among different nodes, unfortunately, and devm_ioremap_resource
>> will fail.
>
> This sounds like it should be fixed in the DT binding then, to ensure
> that the ranges don't overlap.
>
> Mapping the same register region multiple times is generally considered
> a bad idea because the drivers that map them often don't have global
> locks that serialize the access, so it's better to have code in place
> that ensures that they are distinct.
>
> What is the purpose of the ctrl_regs region, and why is it shared
> across multiple devices?
>
> Are all users of these registers in the same driver?
>

We are considering using syscon for ctrl_regs.

Thanks Arnd.

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[Arnd Bergmann]

On Monday 12 October 2015 23:20:25 John Garry wrote:
> @@ -804,6 +818,16 @@ static irqreturn_t int_phyup_v1_hw(int irq_no, void *p)
>                 phy->identify.target_port_protocols =
>                         SAS_PROTOCOL_SMP;
>  
> +       wq = kmalloc(sizeof(*wq), GFP_ATOMIC);
> +       if (!wq)
> +               goto end;
> +
> +       wq->event = PHYUP;
> +       wq->hisi_hba = hisi_hba;
> +       wq->phy_no = phy_no;
> +
> +       INIT_WORK(&wq->work_struct, hisi_sas_wq_process);
> +       queue_work(hisi_hba->wq, &wq->work_struct);
>  
>  end:
>         hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2,
> 

While rereading some other parts of the code, I stumbled over this piece.
You should generally not allocate work structs dynamically. Why not embed
the work struct inside of the phy structure and then just queue that?

	Arnd

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[John Garry]

On 16/10/2015 13:55, Arnd Bergmann wrote:
> On Monday 12 October 2015 23:20:25 John Garry wrote:
>> @@ -804,6 +818,16 @@ static irqreturn_t int_phyup_v1_hw(int irq_no, void *p)
>>                  phy->identify.target_port_protocols =
>>                          SAS_PROTOCOL_SMP;
>>
>> +       wq = kmalloc(sizeof(*wq), GFP_ATOMIC);
>> +       if (!wq)
>> +               goto end;
>> +
>> +       wq->event = PHYUP;
>> +       wq->hisi_hba = hisi_hba;
>> +       wq->phy_no = phy_no;
>> +
>> +       INIT_WORK(&wq->work_struct, hisi_sas_wq_process);
>> +       queue_work(hisi_hba->wq, &wq->work_struct);
>>
>>   end:
>>          hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2,
>>
>
> While rereading some other parts of the code, I stumbled over this piece.
> You should generally not allocate work structs dynamically. Why not embed
> the work struct inside of the phy structure and then just queue that?
>
> 	Arnd
>
> .
>

It could be considered.

A potential issue I see is with hisi_sas_control_phy() for 
PHY_FUNC_HARD_RESET: this allocates a hisi_sas_wq struct and processes 
the reset in the queue work. When we re-enable the phy for the reset, 
the phyup irq will want to use the same hisi_sas_wq struct which may be 
in use.

hisi_sas_control_phy() is added in 23/35.

John

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[Arnd Bergmann]

On Friday 16 October 2015 14:29:55 John Garry wrote:
> 
> It could be considered.
> 
> A potential issue I see is with hisi_sas_control_phy() for 
> PHY_FUNC_HARD_RESET: this allocates a hisi_sas_wq struct and processes 
> the reset in the queue work. When we re-enable the phy for the reset, 
> the phyup irq will want to use the same hisi_sas_wq struct which may be 
> in use.
> 
> hisi_sas_control_phy() is added in 23/35.

I'd have to review more closely, but I think that's fine, as this
is how most work queues are used: you can queue the same function
multiple times, and it's guaranteed to run at least once after
the last queue, so if you queue it while it's already running,
it will be called again, otherwise it won't.

	Arnd

Re: [PATCH 00/25] HiSilicon SAS driver

[John Garry]

Hi James,

Could we please get a review for our driver? We have got some good input 
on changes we need to make, and we will produce another patchset in the 
coming days. However it would good to get a subsystem maintainer 
review/acknowledgement to progress our upstreaming.

Thanks in advance,
John

On 12/10/2015 16:20, John Garry wrote:
> This is the driver patchset for the HiSilicon SAS driver. The driver
> is a platform driver.
>
> The driver will support multiple revisions of HW. Currently only "v1"
> HW is supported.
>
> The driver uses libsas framework within the SCSI framework.
>
> The v1 HW supports SSP and SMP, but not STP/SATA.
>
>
> John Garry (25):
>    [SCSI] sas: centralise ssp frame information units
>    devicetree: bindings: scsi: HiSi SAS
>    scsi: hisi_sas: add initial bare driver
>    scsi: hisi_sas: add scsi host registration
>    scsi: hisi_sas: allocate memories and create pools
>    scsi: hisi_sas: add slot init code
>    scsi: hisi_sas: add ioremap for device HW
>    scsi: hisi_sas: add cq structure initialization
>    scsi: hisi_sas: add phy SAS ADDR initialization
>    scsi: hisi_sas: add misc HBA initialization
>    scsi: hisi_sas: add v1 hardware register definitions
>    scsi: hisi_sas: add v1 HW initialisation code
>    scsi: hisi_sas: add path from phyup irq to SAS framework
>    scsi: hisi_sas: add ssp command function
>    scsi: hisi_sas: add cq interrupt handler
>    scsi: hisi_sas: add dev_found and port_formed
>    scsi: hisi_sas: add abnormal irq handler
>    scsi: hisi_sas: add dev_gone and port_deformed
>    scsi: hisi_sas: add bcast interrupt handler
>    scsi: hisi_sas: add smp protocol support
>    scsi: hisi_sas: add scan finished and start
>    scsi: hisi_sas: add tmf methods
>    scsi: hisi_sas: add control phy handler
>    scsi: hisi_sas: add fatal irq handler
>    MAINTAINERS: add maintainer for HiSi SAS driver
>
>   .../devicetree/bindings/scsi/hisilicon-sas.txt     |   63 +
>   MAINTAINERS                                        |    7 +
>   drivers/scsi/Kconfig                               |    1 +
>   drivers/scsi/Makefile                              |    1 +
>   drivers/scsi/aic94xx/aic94xx_sas.h                 |   49 +-
>   drivers/scsi/hisi_sas/Kconfig                      |    5 +
>   drivers/scsi/hisi_sas/Makefile                     |    2 +
>   drivers/scsi/hisi_sas/hisi_sas.h                   |  406 +++++
>   drivers/scsi/hisi_sas/hisi_sas_init.c              |  489 ++++++
>   drivers/scsi/hisi_sas/hisi_sas_main.c              | 1115 ++++++++++++
>   drivers/scsi/hisi_sas/hisi_sas_v1_hw.c             | 1850 ++++++++++++++++++++
>   include/scsi/sas.h                                 |   74 +
>   12 files changed, 4019 insertions(+), 43 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/scsi/hisilicon-sas.txt
>   create mode 100644 drivers/scsi/hisi_sas/Kconfig
>   create mode 100644 drivers/scsi/hisi_sas/Makefile
>   create mode 100644 drivers/scsi/hisi_sas/hisi_sas.h
>   create mode 100644 drivers/scsi/hisi_sas/hisi_sas_init.c
>   create mode 100644 drivers/scsi/hisi_sas/hisi_sas_main.c
>   create mode 100644 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
>

Re: [PATCH 00/25] HiSilicon SAS driver

[Hannes Reinecke]

On 10/19/2015 10:47 AM, John Garry wrote:
> Hi James,
> 
> Could we please get a review for our driver? We have got some good
> input on changes we need to make, and we will produce another
> patchset in the coming days. However it would good to get a
> subsystem maintainer review/acknowledgement to progress our
> upstreaming.
> 
In general any change will need to have at least two reviews, of
which one should be independent (ie not affiliated with the company)
from the original submitter.

Once the reviews are done the maintainer will/should do a final
round of reviews and then include the patch in the next submission.

However, you seem to be on a good course with your driver.
Please prepare for the next round of patches and we'll be reviewing
them.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 00/25] HiSilicon SAS driver

[John Garry]

On 19/10/2015 09:55, Hannes Reinecke wrote:
> On 10/19/2015 10:47 AM, John Garry wrote:
>> Hi James,
>>
>> Could we please get a review for our driver? We have got some good
>> input on changes we need to make, and we will produce another
>> patchset in the coming days. However it would good to get a
>> subsystem maintainer review/acknowledgement to progress our
>> upstreaming.
>>
> In general any change will need to have at least two reviews, of
> which one should be independent (ie not affiliated with the company)
> from the original submitter.
>
> Once the reviews are done the maintainer will/should do a final
> round of reviews and then include the patch in the next submission.
>
> However, you seem to be on a good course with your driver.
> Please prepare for the next round of patches and we'll be reviewing
> them.
>
> Cheers,
>
> Hannes
>

OK, thanks. I just want to make sure we're on the right path.

Regards,
john

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[John Garry]

On 16/10/2015 14:36, Arnd Bergmann wrote:
> On Friday 16 October 2015 14:29:55 John Garry wrote:
>>
>> It could be considered.
>>
>> A potential issue I see is with hisi_sas_control_phy() for
>> PHY_FUNC_HARD_RESET: this allocates a hisi_sas_wq struct and processes
>> the reset in the queue work. When we re-enable the phy for the reset,
>> the phyup irq will want to use the same hisi_sas_wq struct which may be
>> in use.
>>
>> hisi_sas_control_phy() is added in 23/35.
>
> I'd have to review more closely, but I think that's fine, as this
> is how most work queues are used: you can queue the same function
> multiple times, and it's guaranteed to run at least once after
> the last queue, so if you queue it while it's already running,
> it will be called again, otherwise it won't.
>
> 	Arnd
>
> .
>
In the scenario I described the issue is not that the second call to 
queue the work function is lost. The problem is that when we setup the 
second call we may overwrite elements of the phy's hisi_sas_wq struct 
which may be still being referenced in the work function for the first call.

Regards,
John

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[Arnd Bergmann]

On Monday 19 October 2015 15:11:47 John Garry wrote:
> On 16/10/2015 14:36, Arnd Bergmann wrote:
> > On Friday 16 October 2015 14:29:55 John Garry wrote:
> >>
> >> It could be considered.
> >>
> >> A potential issue I see is with hisi_sas_control_phy() for
> >> PHY_FUNC_HARD_RESET: this allocates a hisi_sas_wq struct and processes
> >> the reset in the queue work. When we re-enable the phy for the reset,
> >> the phyup irq will want to use the same hisi_sas_wq struct which may be
> >> in use.
> >>
> >> hisi_sas_control_phy() is added in 23/35.
> >
> > I'd have to review more closely, but I think that's fine, as this
> > is how most work queues are used: you can queue the same function
> > multiple times, and it's guaranteed to run at least once after
> > the last queue, so if you queue it while it's already running,
> > it will be called again, otherwise it won't.
> >
> In the scenario I described the issue is not that the second call to 
> queue the work function is lost. The problem is that when we setup the 
> second call we may overwrite elements of the phy's hisi_sas_wq struct 
> which may be still being referenced in the work function for the first call.

Do you mean these members?

> +       wq->event = PHYUP;
> +       wq->hisi_hba = hisi_hba;
> +       wq->phy_no = phy_no;

Sorry for being unclear, I implied getting rid of them, by having a
work queue per phy. You can create a structure for each phy like

	struct hisi_sas_phy {
		struct hisi_sas *dev; /* pointer to the device */
		unsigned int phy_no;
		struct work_struct phyup_work;
	};

There are probably other things you can put into the same structure.
When the phy is brought up, you then just start the phyup work for
that phy, which can retrieve its hisi_sas_phy structure from the
work_struct pointer, and from there get to the device.

	Arnd

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[John Garry]

>>> I'd have to review more closely, but I think that's fine, as this
>>> is how most work queues are used: you can queue the same function
>>> multiple times, and it's guaranteed to run at least once after
>>> the last queue, so if you queue it while it's already running,
>>> it will be called again, otherwise it won't.
>>>
>> In the scenario I described the issue is not that the second call to
>> queue the work function is lost. The problem is that when we setup the
>> second call we may overwrite elements of the phy's hisi_sas_wq struct
>> which may be still being referenced in the work function for the first call.
>
> Do you mean these members?
>
>> +       wq->event = PHYUP;
>> +       wq->hisi_hba = hisi_hba;
>> +       wq->phy_no = phy_no;
>
Yes, these are the members I was referring to. However there is also an 
element "data" in hisi_sas_wq. This is used in control phy as follows:
int hisi_sas_control_phy(struct asd_sas_phy *sas_phy,
			enum phy_func func,
			void *funcdata)
{
	...
	wq->event = CONTROL_PHY;
	wq->data = func;
	...
	INIT_WORK(&wq->work_struct, hisi_sas_wq_process);
	queue_work(hisi_hba->wq, &wq->work_struct);
}

void hisi_sas_wq_process(struct work_struct *work)
{
	struct hisi_sas_wq *wq =
		container_of(work, struct hisi_sas_wq, work_struct);
	switch (wq->event) {
	case CONTROL_PHY:
		hisi_sas_control_phy_work(hisi_hba, wq->data, phy_no);
}

static void hisi_sas_control_phy_work(struct hisi_hba *hisi_hba,
				int func,
				int phy_no)
{
	switch (func) {
	case PHY_FUNC_HARD_RESET:
		hard_phy_reset_v1_hw(hisi_hba, phy_no)

> Sorry for being unclear, I implied getting rid of them, by having a
> work queue per phy. You can create a structure for each phy like
>
> 	struct hisi_sas_phy {
> 		struct hisi_sas *dev; /* pointer to the device */
> 		unsigned int phy_no;
> 		struct work_struct phyup_work;
> 	};
>
> There are probably other things you can put into the same structure.
> When the phy is brought up, you then just start the phyup work for
> that phy, which can retrieve its hisi_sas_phy structure from the
> work_struct pointer, and from there get to the device.
>
> 	Arnd
>
> .
>
We could create a work_struct for each event, which would be fine. 
However we would sometimes still need some way of passing data to the 
event, like the phy control example.

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[Arnd Bergmann]

On Monday 19 October 2015 15:55:47 John Garry wrote:
> > Do you mean these members?
> >
> >> +       wq->event = PHYUP;
> >> +       wq->hisi_hba = hisi_hba;
> >> +       wq->phy_no = phy_no;
> >
> Yes, these are the members I was referring to. However there is also an 
> element "data" in hisi_sas_wq. This is used in control phy as follows:
> int hisi_sas_control_phy(struct asd_sas_phy *sas_phy,
>                         enum phy_func func,
>                         void *funcdata)
> {
>         ...
>         wq->event = CONTROL_PHY;
>         wq->data = func;
>         ...
>         INIT_WORK(&wq->work_struct, hisi_sas_wq_process);
>         queue_work(hisi_hba->wq, &wq->work_struct);
> }
> 
> void hisi_sas_wq_process(struct work_struct *work)
> {
>         struct hisi_sas_wq *wq =
>                 container_of(work, struct hisi_sas_wq, work_struct);
>         switch (wq->event) {
>         case CONTROL_PHY:
>                 hisi_sas_control_phy_work(hisi_hba, wq->data, phy_no);
> }
> 
> static void hisi_sas_control_phy_work(struct hisi_hba *hisi_hba,
>                                 int func,
>                                 int phy_no)
> {
>         switch (func) {
>         case PHY_FUNC_HARD_RESET:
>                 hard_phy_reset_v1_hw(hisi_hba, phy_no)

Ok.

> > Sorry for being unclear, I implied getting rid of them, by having a
> > work queue per phy. You can create a structure for each phy like
> >
> >       struct hisi_sas_phy {
> >               struct hisi_sas *dev; /* pointer to the device */
> >               unsigned int phy_no;
> >               struct work_struct phyup_work;
> >       };
> >
> > There are probably other things you can put into the same structure.
> > When the phy is brought up, you then just start the phyup work for
> > that phy, which can retrieve its hisi_sas_phy structure from the
> > work_struct pointer, and from there get to the device.
> >
> We could create a work_struct for each event, which would be fine.

Yes, that would be the normal way to do it. You initialize the work
structures from the initial probe function to have the right callbacks,
and then you just queue the right one when you need to defer an event.

 How many different events are there?

> However we would sometimes still need some way of passing data to the 
> event, like the phy control example.

Do you mean the 'int func' argument to hisi_sas_control_phy_work?
It sounds like that would again just be more different work_structs.

At some point that might get silly (having 10 or more work structs
per phy), but then you could restructure the code to use something
other that work queues to get from interrupt context to process
context, e.g. a threaded interrupt handler.

Note that the current code is not only unusual but also fragile
because you rely on GFP_ATOMIC memory allocations from the interrupt
handler, and they tend to eventually fail.

	Arnd

Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework

[John Garry]

>> We could create a work_struct for each event, which would be fine.
>
> Yes, that would be the normal way to do it. You initialize the work
> structures from the initial probe function to have the right
> callbacks, and then you just queue the right one when you need to
> defer an event.
>
> How many different events are there?
>
We currently only support processing 2 events in the workqueue: phy up 
and control phy. However we may want to use more in the future, like
hotplug (for phy down) event.

>> However we would sometimes still need some way of passing data to
>> the event, like the phy control example.
>
> Do you mean the 'int func' argument to hisi_sas_control_phy_work?
Yes
> It sounds like that would again just be more different work_structs.
>
> At some point that might get silly (having 10 or more work structs
> per phy), but then you could restructure the code to use something
> other that work queues to get from interrupt context to process
> context, e.g. a threaded interrupt handler.
I'll check on this. We need to consider how to pass the argument for the 
control phy case.
>
> Note that the current code is not only unusual but also fragile
> because you rely on GFP_ATOMIC memory allocations from the interrupt
> handler, and they tend to eventually fail.
Understood. For what it's worth, I was just following other SAS drivers
as a refernce: see pm8001_handle_event() and mvs_handle_event()

>
> Arnd -- To unsubscribe from this list: send the line "unsubscribe
> linux-scsi" in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> .
>
Thanks, John