| Message ID | 1444663237-238302-14-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi John,
[auto build test WARNING on scsi/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/John-Garry/HiSilicon-SAS-driver/20151012-231929
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
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
Hi kbuild test robot people, On Tue, Oct 13, 2015 at 9:03 AM, kbuild test robot <lkp@intel.com> wrote: > Hi John, > > [auto build test WARNING on scsi/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base] > > url: https://github.com/0day-ci/linux/commits/John-Garry/HiSilicon-SAS-driver/20151012-231929 > reproduce: > # apt-get install sparse > make ARCH=x86_64 allmodconfig > make C=1 CF=-D__CHECK_ENDIAN__ > > > sparse warnings: (new ones prefixed by >>) Your script seems to have lost the actual warnings here. > Please review and possibly fold the followup patch. Thanks,
Hi Julian, Oops, this is awkward.. I'll check it, thank you for the reminder! Thanks, Fengguang On Tue, Oct 13, 2015 at 11:27:05AM +1100, Julian Calaby wrote: > Hi kbuild test robot people, > > On Tue, Oct 13, 2015 at 9:03 AM, kbuild test robot <lkp@intel.com> wrote: > > Hi John, > > > > [auto build test WARNING on scsi/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base] > > > > url: https://github.com/0day-ci/linux/commits/John-Garry/HiSilicon-SAS-driver/20151012-231929 > > reproduce: > > # apt-get install sparse > > make ARCH=x86_64 allmodconfig > > make C=1 CF=-D__CHECK_ENDIAN__ > > > > > > sparse warnings: (new ones prefixed by >>) > > Your script seems to have lost the actual warnings here. > > > Please review and possibly fold the followup patch. > > Thanks, > > -- > Julian Calaby > > Email: julian.calaby@gmail.com > Profile: http://www.google.com/profiles/julian.calaby/ -- 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
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 -- 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
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 -- 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
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 -- 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
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> 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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h index 18fd30b..802c000 100644 --- a/drivers/scsi/hisi_sas/hisi_sas.h +++ b/drivers/scsi/hisi_sas/hisi_sas.h @@ -333,4 +333,5 @@ extern int interrupt_init_v1_hw(struct hisi_hba *hisi_hba); extern int interrupt_openall_v1_hw(struct hisi_hba *hisi_hba); extern int hw_init_v1_hw(struct hisi_hba *hisi_hba); extern int phys_init_v1_hw(struct hisi_hba *hisi_hba); +extern void sl_notify_v1_hw(struct hisi_hba *hisi_hba, int phy_no); #endif diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c index 882ff79..72831db 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_main.c +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c @@ -27,10 +27,65 @@ void hisi_sas_slot_index_init(struct hisi_hba *hisi_hba) hisi_sas_slot_index_clear(hisi_hba, i); } + +void hisi_sas_bytes_dmaed(struct hisi_hba *hisi_hba, int phy_no) +{ + struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no]; + struct asd_sas_phy *sas_phy = &phy->sas_phy; + struct sas_ha_struct *sas_ha; + + if (!phy->phy_attached) + return; + + sas_ha = &hisi_hba->sha; + sas_ha->notify_phy_event(sas_phy, PHYE_OOB_DONE); + + if (sas_phy->phy) { + struct sas_phy *sphy = sas_phy->phy; + + sphy->negotiated_linkrate = sas_phy->linkrate; + sphy->minimum_linkrate = phy->minimum_linkrate; + sphy->minimum_linkrate_hw = SAS_LINK_RATE_1_5_GBPS; + sphy->maximum_linkrate = phy->maximum_linkrate; + } + + if (phy->phy_type & PORT_TYPE_SAS) { + struct sas_identify_frame *id; + + id = (struct sas_identify_frame *)phy->frame_rcvd; + id->dev_type = phy->identify.device_type; + id->initiator_bits = SAS_PROTOCOL_ALL; + id->target_bits = phy->identify.target_port_protocols; + } else if (phy->phy_type & PORT_TYPE_SATA) { + /*Nothing*/ + } + + sas_phy->frame_rcvd_size = phy->frame_rcvd_size; + + sas_ha->notify_port_event(sas_phy, PORTE_BYTES_DMAED); +} + + +static void hisi_sas_phyup_work(struct hisi_hba *hisi_hba, + int phy_no) +{ + sl_notify_v1_hw(hisi_hba, phy_no); /* This requires a sleep */ + hisi_sas_bytes_dmaed(hisi_hba, phy_no); +} + void hisi_sas_wq_process(struct work_struct *work) { struct hisi_sas_wq *wq = container_of(work, struct hisi_sas_wq, work_struct); + struct hisi_hba *hisi_hba = wq->hisi_hba; + int event = wq->event; + int phy_no = wq->phy_no; + + switch (event) { + case PHYUP: + hisi_sas_phyup_work(hisi_hba, phy_no); + break; + } kfree(wq); } diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c index 20291b4..df56b3a 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c @@ -738,6 +738,19 @@ int phys_init_v1_hw(struct hisi_hba *hisi_hba) return 0; } +void sl_notify_v1_hw(struct hisi_hba *hisi_hba, int phy_no) +{ + u32 sl_control; + + sl_control = hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL); + sl_control |= SL_CONTROL_NOTIFY_EN_MSK; + hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control); + msleep(1); + sl_control = hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL); + sl_control &= ~SL_CONTROL_NOTIFY_EN_MSK; + hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control); +} + /* Interrupts */ static irqreturn_t int_phyup_v1_hw(int irq_no, void *p) { @@ -750,6 +763,7 @@ static irqreturn_t int_phyup_v1_hw(int irq_no, void *p) struct sas_identify_frame *id = (struct sas_identify_frame *)frame_rcvd; u32 irq_value, context, port_id, link_rate; int i; + struct hisi_sas_wq *wq = NULL; irqreturn_t res = IRQ_HANDLED; irq_value = hisi_sas_phy_read32(hisi_hba, phy_no, CHL_INT2); @@ -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,
Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/hisi_sas/hisi_sas.h | 1 + drivers/scsi/hisi_sas/hisi_sas_main.c | 55 ++++++++++++++++++++++++++++++++++ drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 24 +++++++++++++++ 3 files changed, 80 insertions(+)