Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1250738 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS  framework John Garry <john.garry@huawei.com> - 2015-10-19 16:20 +0200
    Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework Arnd Bergmann <arnd@arndb.de> - 2015-10-19 16:30 +0200
      Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS  framework John Garry <john.garry@huawei.com> - 2015-10-19 17:00 +0200
        Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS framework Arnd Bergmann <arnd@arndb.de> - 2015-10-20 10:50 +0200
          Re: [PATCH 13/25] scsi: hisi_sas: add path from phyup irq to SAS  framework John Garry <john.garry@huawei.com> - 2015-10-20 11:30 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1250767

>>> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251501

>> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web