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. 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. > + 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. > +{ > + 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. -Tyrel
