Tyrel Datwyler <[email protected]> writes: > On 4/8/26 10:07 AM, Dave Marquardt via B4 Relay wrote: >> From: Dave Marquardt <[email protected]> >> >> - Add FPIN event descriptor >> - Add congestion cleared status >> - Add code to handle basic FPIN async event >> - Add KUnit tests > > You need a more detailed description of your changes here for the commit log > body. > > You will also need a signed off tag from yourself for this to even be merged.
Odd. I used b4 to prepare the patch set, and it should have added a Signed-off-by: tag. I'll double check it next round. > https://www.kernel.org/doc/html/latest/process/submitting-patches.html > >> --- >> drivers/scsi/Kconfig | 10 ++ >> drivers/scsi/ibmvscsi/Makefile | 1 + >> drivers/scsi/ibmvscsi/ibmvfc.c | 189 >> ++++++++++++++++++++++++++++++++++- >> drivers/scsi/ibmvscsi/ibmvfc.h | 9 ++ >> drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 95 ++++++++++++++++++ >> 5 files changed, 302 insertions(+), 2 deletions(-) > > <snip> > >> +static struct fc_els_fpin * >> +ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64 wwpn, __be16 modifier, >> + __be32 period, __be32 threshold, __be32 event_count) >> +{ >> + struct fc_fn_peer_congn_desc *pdesc; >> + struct fc_fn_congn_desc *cdesc; >> + struct fc_fn_li_desc *ldesc; >> + struct fc_els_fpin *fpin; >> + size_t size; >> + >> + size = ibmvfc_fpin_size_helper(fpin_status); >> + if (size == 0) >> + return NULL; >> + >> + fpin = kzalloc(size, GFP_KERNEL); > > This appears to be called by ibmvfc_handle_async() with runs in atomic context > and cannot therefore sleep. This allocation needs to be GFP_ATOMIC. Although > there is another issue below that might make this moot. Noted. As to the problem below, once this is moved to running in a work queue worker thread, this can stay as is. >> + if (fpin == NULL) >> + return NULL; >> + >> + fpin->fpin_cmd = ELS_FPIN; >> + >> + switch (fpin_status) { >> + case IBMVFC_AE_FPIN_CONGESTION_CLEARED: >> + case IBMVFC_AE_FPIN_LINK_CONGESTED: >> + fpin->desc_len = cpu_to_be32(sizeof(struct fc_fn_congn_desc)); >> + cdesc = (struct fc_fn_congn_desc *)fpin->fpin_desc; >> + cdesc->desc_tag = cpu_to_be32(ELS_DTAG_CONGESTION); >> + cdesc->desc_len = >> cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*cdesc)); >> + if (fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED) >> + cdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR); >> + else >> + cdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC); >> + cdesc->event_modifier = modifier; >> + cdesc->event_period = period; >> + cdesc->severity = FPIN_CONGN_SEVERITY_WARNING; >> + break; >> + case IBMVFC_AE_FPIN_PORT_CONGESTED: >> + case IBMVFC_AE_FPIN_PORT_CLEARED: >> + fpin->desc_len = cpu_to_be32(sizeof(struct >> fc_fn_peer_congn_desc)); >> + pdesc = (struct fc_fn_peer_congn_desc *)fpin->fpin_desc; >> + pdesc->desc_tag = cpu_to_be32(ELS_DTAG_PEER_CONGEST); >> + pdesc->desc_len = >> cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*pdesc)); >> + if (fpin_status == IBMVFC_AE_FPIN_PORT_CLEARED) >> + pdesc->event_type = cpu_to_be16(FPIN_CONGN_CLEAR); >> + else >> + pdesc->event_type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC); >> + pdesc->event_modifier = modifier; >> + pdesc->event_period = period; >> + pdesc->detecting_wwpn = cpu_to_be64(0); >> + pdesc->attached_wwpn = wwpn; >> + pdesc->pname_count = cpu_to_be32(1); >> + pdesc->pname_list[0] = wwpn; >> + break; >> + case IBMVFC_AE_FPIN_PORT_DEGRADED: >> + fpin->desc_len = cpu_to_be32(sizeof(struct fc_fn_li_desc)); >> + ldesc = (struct fc_fn_li_desc *)fpin->fpin_desc; >> + ldesc->desc_tag = cpu_to_be32(ELS_DTAG_LNK_INTEGRITY); >> + ldesc->desc_len = >> cpu_to_be32(FC_TLV_DESC_LENGTH_FROM_SZ(*ldesc)); >> + ldesc->event_type = cpu_to_be16(FPIN_LI_UNKNOWN); >> + ldesc->event_modifier = modifier; >> + ldesc->event_threshold = threshold; >> + ldesc->event_count = event_count; >> + ldesc->detecting_wwpn = cpu_to_be64(0); >> + ldesc->attached_wwpn = wwpn; >> + ldesc->pname_count = cpu_to_be32(1); >> + ldesc->pname_list[0] = wwpn; >> + break; >> + default: >> + /* This should be caught above. */ >> + kfree(fpin); >> + fpin = NULL; >> + break; >> + } >> + >> + return fpin; >> +} >> + >> +/** >> + * ibmvfc_basic_fpin_to_desc(): allocate and populate a struct fc_els_fpin >> struct >> + * containing a descriptor. >> + * @ibmvfc_fpin: Pointer to async crq >> + * >> + * Allocate a struct fc_els_fpin containing a descriptor and populate >> + * based on data from *ibmvfc_fpin. >> + * >> + * Return: >> + * NULL - unable to allocate structure >> + * non-NULL - pointer to populated struct fc_els_fpin >> + */ >> +static struct fc_els_fpin * >> +/*XXX*/ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq) > > What is with this /*XXX*/? I can't find it once I apply the patchset so I > assume > its removed in a later patch, but it should be removed here. An artifact I missed in squashing commits. Thanks for pointing it out. >> +{ >> + return ibmvfc_common_fpin_to_desc(crq->fpin_status, crq->wwpn, >> + cpu_to_be16(0), >> + >> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_PERIOD), >> + >> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD), >> + cpu_to_be32(1)); >> +} >> + >> /** >> * ibmvfc_handle_async - Handle an async event from the adapter >> * @crq: crq to process >> * @vhost: ibmvfc host struct >> * >> **/ >> -static void ibmvfc_handle_async(struct ibmvfc_async_crq *crq, >> - struct ibmvfc_host *vhost) >> +VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq, >> + struct ibmvfc_host *vhost) >> { >> const struct ibmvfc_async_desc *desc = >> ibmvfc_get_ae_desc(be64_to_cpu(crq->event)); >> struct ibmvfc_target *tgt; >> + struct fc_els_fpin *fpin; >> >> ibmvfc_log(vhost, desc->log_level, "%s event received. scsi_id: %llx, >> wwpn: %llx," >> " node_name: %llx%s\n", desc->desc, >> be64_to_cpu(crq->scsi_id), >> @@ -3269,11 +3422,37 @@ static void ibmvfc_handle_async(struct >> ibmvfc_async_crq *crq, >> case IBMVFC_AE_HALT: >> ibmvfc_link_down(vhost, IBMVFC_HALTED); >> break; >> + case IBMVFC_AE_FPIN: >> + if (!crq->scsi_id && !crq->wwpn && !crq->node_name) >> + break; >> + list_for_each_entry(tgt, &vhost->targets, queue) { >> + if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != >> crq->scsi_id) >> + continue; >> + if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != >> crq->wwpn) >> + continue; >> + if (crq->node_name && cpu_to_be64(tgt->ids.node_name) >> != crq->node_name) >> + continue; >> + if (!tgt->rport) >> + continue; >> + fpin = ibmvfc_basic_fpin_to_desc(crq); >> + if (fpin) { >> + fc_host_fpin_rcv(tgt->vhost->host, >> + sizeof(*fpin) + >> + >> be32_to_cpu(fpin->desc_len), >> + (char *)fpin, 0); > > This call to fc_host_fpin_rcv() appears to be problematic as it assumes no > locks > are held, but ibmvfc_handle_async() is called with the scsi host lock held. We > already do a lot more work than we probaly should in our interrupt handler. I > think we maybe need to pass the FPIN work off to a workqueue instead to be > handled in process context instead. Agreed. I'm working on adding a work queue for offloading this FPIN processing. -Dave
