Tyrel Datwyler <[email protected]> writes:
> On 6/8/26 11:30 AM, Dave Marquardt via B4 Relay wrote:
>> From: Dave Marquardt <[email protected]>
>>
>> Refactor existing code for async events into a common routine,
>> register a channel and interrupt handler for the asynchronous
>> sub-queue, and set capability bits to request that VIOS use the
>> asynchronous sub-queue.
>
> Again, all your patches need Signed-off-by: tags to be accepted.
>
> Also, there is still a lot of different things happening in this patch
>
> You seem to be reworking the fpin_desc helpers which I'm not really seeing any
> reason that you couldn't squash those changes into patch 2. It seems like
> needless churn since, unless I'm missing something, there doesn't seem to be
> any
> new definitions added since patch 2 just reworking of the input parameters
> such
> that period goes away and is hardcoded and a new type paramerter is added.
Okay, I'll roll the ibmvfc_common_fpin_to_desc changes into patch 2.
> I would also help reduce the patch size and make it more reviewable if you
> split
> the the interrupt handler definition and queue registration into separate
> patches as well.
>
>> ---
>> drivers/scsi/ibmvscsi/ibmvfc.c | 376
>> +++++++++++++++++++++++++++--------
>> drivers/scsi/ibmvscsi/ibmvfc.h | 3 +
>> drivers/scsi/ibmvscsi/ibmvfc_kunit.c | 2 +-
>> 3 files changed, 298 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
>> index ad1f5636e879..a2252cd2f44b 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
>> @@ -1514,7 +1514,8 @@ static void ibmvfc_set_login_info(struct ibmvfc_host
>> *vhost)
>> login_info->max_cmds = cpu_to_be32(max_cmds);
>> login_info->capabilities =
>> cpu_to_be64(IBMVFC_CAN_MIGRATE | IBMVFC_CAN_SEND_VF_WWPN |
>> - IBMVFC_CAN_USE_NOOP_CMD);
>> + IBMVFC_CAN_USE_NOOP_CMD | IBMVFC_YES_SCSI |
>> + IBMVFC_USE_ASYNC_SUBQ | IBMVFC_CAN_HANDLE_FPIN);
>>
>> if (vhost->mq_enabled || vhost->using_channels)
>> login_info->capabilities |=
>> cpu_to_be64(IBMVFC_CAN_USE_CHANNELS);
>> @@ -3240,8 +3241,8 @@ static size_t ibmvfc_fpin_size_helper(u8 fpin_status)
>> * non-NULL - pointer to populated struct fc_els_fpin
>> */
>> static struct fc_els_fpin *
>> -ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64 wwpn, __be16 modifier,
>> - __be32 period, __be32 threshold, __be32 event_count)
>> +ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64 wwpn, __be16 type, __be16
>> modifier,
>> + __be32 threshold, __be32 event_count)
>> {
>
> This function signature changes here, and looking at the implemenation below
> I'm
> struggling to see why this couldn't just be all implmented immediatly in
> Patch #2.
>
>> struct fc_fn_peer_congn_desc *pdesc;
>> struct fc_fn_congn_desc *cdesc;
>> @@ -3253,7 +3254,7 @@ ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64
>> wwpn, __be16 modifier,
>> if (size == 0)
>> return NULL;
>>
>> - fpin = kzalloc(size, GFP_KERNEL);
>> + fpin = kzalloc(size, GFP_ATOMIC);
>
> Why are we changing this to GFP_ATOMIC? Isn't this only ever called from work
> queue context?
I've restored this for my next version of the patch series.
>> if (fpin == NULL)
>> return NULL;
>>
>> @@ -3266,12 +3267,9 @@ ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64
>> wwpn, __be16 modifier,
>> 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_type = type;
>> cdesc->event_modifier = modifier;
>> - cdesc->event_period = period;
>> + cdesc->event_period =
>> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_PERIOD);
>> cdesc->severity = FPIN_CONGN_SEVERITY_WARNING;
>> break;
>> case IBMVFC_AE_FPIN_PORT_CONGESTED:
>> @@ -3281,12 +3279,9 @@ ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64
>> wwpn, __be16 modifier,
>> 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_type = type;
>> pdesc->event_modifier = modifier;
>> - pdesc->event_period = period;
>> + pdesc->event_period =
>> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_PERIOD);
>> pdesc->detecting_wwpn = cpu_to_be64(0);
>> pdesc->attached_wwpn = wwpn;
>> pdesc->pname_count = cpu_to_be32(1);
>> @@ -3297,7 +3292,7 @@ ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64
>> wwpn, __be16 modifier,
>> 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_type = type;
>> ldesc->event_modifier = modifier;
>> ldesc->event_threshold = threshold;
>> ldesc->event_count = event_count;
>> @@ -3331,9 +3326,47 @@ ibmvfc_common_fpin_to_desc(u8 fpin_status, __be64
>> wwpn, __be16 modifier,
>> static struct fc_els_fpin *
>> ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, u64 wwpn)
>> {
>> + __be16 type;
>> +
>> + switch (crq->fpin_status) {
>> + case IBMVFC_AE_FPIN_LINK_CONGESTED:
>> + case IBMVFC_AE_FPIN_PORT_CONGESTED:
>> + type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
>> + break;
>> + case IBMVFC_AE_FPIN_PORT_CLEARED:
>> + case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
>> + type = cpu_to_be16(FPIN_CONGN_CLEAR);
>> + break;
>> + case IBMVFC_AE_FPIN_PORT_DEGRADED:
>> + type = cpu_to_be16(FPIN_LI_UNKNOWN);
>> + break;
>> + default:
>> + return (NULL);
>> + }
>> +
>> return ibmvfc_common_fpin_to_desc(crq->fpin_status, cpu_to_be64(wwpn),
>> - cpu_to_be16(0),
>> -
>> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_PERIOD),
>> + type, cpu_to_be16(0),
>> +
>> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
>> + cpu_to_be32(1));
>> +}
>> +
>> +/**
>> + * ibmvfc_full_fpin_to_desc(): allocate and populate a struct fc_els_fpin
>> struct
>> + * containing a descriptor.
>> + * @ibmvfc_fpin: Pointer to async subq FPIN data
>> + *
>> + * 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 *
>> +ibmvfc_full_fpin_to_desc(struct ibmvfc_async_subq *ibmvfc_fpin)
>> +{
>> + return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status,
>> ibmvfc_fpin->wwpn,
>> + cpu_to_be16(0), cpu_to_be16(0),
>>
>> cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
>> cpu_to_be32(1));
>> }
>> @@ -3344,67 +3377,99 @@ ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq
>> *crq, u64 wwpn)
>> */
>> static void ibmvfc_process_async_work(struct work_struct *work)
>> {
>> + struct ibmvfc_async_subq_fpin *sqfpin;
>> + struct ibmvfc_target *tgt, *next;
>> + struct ibmvfc_async_subq *subq;
>> struct ibmvfc_async_work *aw;
>> struct ibmvfc_async_crq *crq;
>> - struct ibmvfc_target *tgt;
>> struct ibmvfc_host *vhost;
>> struct fc_els_fpin *fpin;
>> + __be64 node_name;
>> + __be64 scsi_id;
>> + __be64 wwpn;
>>
>> aw = container_of(work, struct ibmvfc_async_work, async_work_s);
>> crq = aw->crq;
>> + subq = aw->subq;
>> vhost = aw->vhost;
>>
>> - if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
>> + if ((!crq && !subq) || (crq && subq)) {
>> + dev_err_ratelimited(vhost->dev,
>> + "FPIN event received, unable to process\n");
>> goto end;
>> - list_for_each_entry(tgt, &vhost->targets, queue) {
>> - if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != crq->scsi_id)
>> + }
>> +
>> + if (crq) {
>> + wwpn = crq->wwpn;
>> + node_name = crq->node_name;
>> + scsi_id = crq->scsi_id;
>> + } else {
>> + wwpn = subq->wwpn;
>> + node_name = subq->id.node_name;
>> + scsi_id = 0;
>> + }
>> +
>> + if (!scsi_id && !wwpn && !node_name)
>> + goto end;
>> +
>> + list_for_each_entry_safe(tgt, next, &vhost->targets, queue) {
>> + if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
>> continue;
>> - if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn)
>> + if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
>> continue;
>> - if (crq->node_name && cpu_to_be64(tgt->ids.node_name) !=
>> crq->node_name)
>> + if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
>> continue;
>> if (!tgt->rport)
>> continue;
>> - fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
>> + if (crq) {
>> + fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
>> + } else {
>> + sqfpin = (struct ibmvfc_async_subq_fpin *)subq;
>> + fpin = ibmvfc_full_fpin_to_desc(subq);
>> + }
>> if (fpin) {
>> fc_host_fpin_rcv(tgt->vhost->host,
>> sizeof(*fpin) +
>> be32_to_cpu(fpin->desc_len),
>> (char *)fpin, 0);
>> kfree(fpin);
>> } else
>> - dev_err_ratelimited(vhost->dev,
>> - "FPIN event %u received, unable to
>> process\n",
>> - crq->fpin_status);
>> + dev_err_ratelimited(vhost->dev, "FPIN event received,
>> unable to process\n");
>> }
>>
>> end:
>> - crq->valid = 0;
>> + if (crq)
>> + crq->valid = 0;
>> + if (subq)
>> + subq->valid = 0;
>>
>> kfree(aw);
>> }
>>
>> /**
>> - * ibmvfc_handle_async - Handle an async event from the adapter
>> - * @crq: crq to process
>> + * ibmvfc_handle_async_common - Handle an async event from the adapter
>> + * @event: event to process
>> + * @link_state: link state
>> * @vhost: ibmvfc host struct
>> + * @scsi_id: scsi_id (0 if not applicable)
>> + * @wwpn: wwpn
>> + * @node_name: node_name
>> + * @aw_crq: crq pointer for async work (NULL if not needed)
>> + * @aw_subq: subq pointer for async work (NULL if not needed)
>> *
>> **/
>> -VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>> - struct ibmvfc_host *vhost)
>> +static void ibmvfc_handle_async_common(u64 event, u8 link_state,
>> + struct ibmvfc_host *vhost,
>> + u64 scsi_id, u64 wwpn, u64 node_name,
>> + struct ibmvfc_async_crq *aw_crq,
>> + struct ibmvfc_async_subq *aw_subq)
>
> This is a lot of parameters which could be extracted here in the function if
> it
> just knew the type of async_crq. Would it be easier to no just take a void
> *async_instance parameter, and an is_subq boolean to determine if you cast the
> void * to ibmvfc_async_crq * or ibmvfc_async_sub_crq *?
Yes, good idea. I've changed this to
VISIBLE_IF_KUNIT void ibmvfc_handle_async(void *crq,
struct ibmvfc_host *vhost,
bool is_subq)
and will extract all of these parameters from crq.
>> +/**
>> + * ibmvfc_handle_async - Handle an async event from the adapter
>> + * @crq: crq to process
>> + * @vhost: ibmvfc host struct
>> + *
>> + **/
>> +VISIBLE_IF_KUNIT void ibmvfc_handle_async(struct ibmvfc_async_crq *crq,
>> + struct ibmvfc_host *vhost)
>
> async and asyncq as in the function name below can be hard to notice the
> difference in name convention. Maybe async_subq would be a better ender, but
> as
> it might be eaiser to just call ibmvfc_handle_async_common(crq, 0) and
> ibmfvc_handle_async(sub_crq, 1), as I outlined above in each of the interrupt
> handlers.
I agree. I've refactored to a singled ibmvfc_handle_async() with a
boolean parameter indicating whether the CRQ is a subqueue CRQ or not.
>> +{
>> + const struct ibmvfc_async_desc *desc =
>> ibmvfc_get_ae_desc(be64_to_cpu(crq->event));
>> + u64 event = be64_to_cpu(crq->event);
>> +
>> + 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),
>> + be64_to_cpu(crq->wwpn), be64_to_cpu(crq->node_name),
>> + ibmvfc_get_link_state(crq->link_state));
>> +
>> + ibmvfc_handle_async_common(event, crq->link_state, vhost,
>> + crq->scsi_id, crq->wwpn, crq->node_name,
>> + crq, NULL);
>> }
>> EXPORT_SYMBOL_IF_KUNIT(ibmvfc_handle_async);
>>
>> +VISIBLE_IF_KUNIT void ibmvfc_handle_asyncq(struct ibmvfc_crq *crq_instance,
>> + struct ibmvfc_host *vhost)
>> +{
>> + struct ibmvfc_async_subq *crq = (struct ibmvfc_async_subq
>> *)crq_instance;
>> + const struct ibmvfc_async_desc *desc =
>> ibmvfc_get_ae_desc(be16_to_cpu(crq->event));
>> + u64 event = be16_to_cpu(crq->event);
>> +
>> + ibmvfc_log(vhost, desc->log_level,
>> + "%s event received. wwpn: %llx, node_name: %llx%s event
>> 0x%x\n",
>> + desc->desc, be64_to_cpu(crq->wwpn),
>> be64_to_cpu(crq->id.node_name),
>> + ibmvfc_get_link_state(crq->link_state),
>> be16_to_cpu(crq->event));
>> +
>> + ibmvfc_handle_async_common(event, crq->link_state, vhost,
>> + 0, crq->wwpn, crq->id.node_name,
>> + NULL, crq);
>> +}
>> +EXPORT_SYMBOL_IF_KUNIT(ibmvfc_handle_asyncq);
>> +
>> /**
>> * ibmvfc_handle_crq - Handles and frees received events in the CRQ
>> * @crq: Command/Response queue
>> @@ -4117,6 +4229,13 @@ static void ibmvfc_handle_scrq(struct ibmvfc_crq
>> *crq, struct ibmvfc_host *vhost
>> spin_unlock(&evt->queue->l_lock);
>> }
>>
>> +/**
>> + * ibmvfc_next_scrq - Returns the next entry in message subqueue
>> + * @scrq: Pointer to message subqueue
>> + *
>> + * Returns:
>> + * Pointer to next entry in queue / NULL if empty
>> + **/
>> static struct ibmvfc_crq *ibmvfc_next_scrq(struct ibmvfc_queue *scrq)
>> {
>> struct ibmvfc_crq *crq;
>> @@ -4132,6 +4251,57 @@ static struct ibmvfc_crq *ibmvfc_next_scrq(struct
>> ibmvfc_queue *scrq)
>> return crq;
>> }
>>
>> +static void ibmvfc_drain_async_subq(struct ibmvfc_queue *scrq)
>> +{
>> + struct ibmvfc_crq *crq;
>> + unsigned long flags;
>> + int done = 0;
>> +
>> + ENTER;
>> +
>> + spin_lock_irqsave(scrq->q_lock, flags);
>> + while (!done) {
>> + while ((crq = ibmvfc_next_scrq(scrq)) != NULL) {
>> + ibmvfc_handle_asyncq(crq, scrq->vhost);
>> + crq->valid = 0;
>
> Isn't up to the handle_async_common to clear the valid crq bit depending on
> the
> type of async action?
I've moved clearing the valid bit to ibmvfc_handle_async().
>> + wmb(); /* complete write */
>> + }
>> +
>> + ibmvfc_toggle_scrq_irq(scrq, 1);
>> + crq = ibmvfc_next_scrq(scrq);
>> + if (crq != NULL) {
>> + ibmvfc_toggle_scrq_irq(scrq, 0);
>> + ibmvfc_handle_asyncq(crq, scrq->vhost);
>> + crq->valid = 0;
>
> Same comment as above.
>
>> + wmb(); /* complete write */
>> + } else
>> + done = 1;
>> + }
>> + spin_unlock_irqrestore(scrq->q_lock, flags);
>> +
>> + LEAVE;
>> +}
>> +
>> +/**
>> + * ibmvfc_interrupt_asyncq - Handle an async event from the adapter
>> + * @irq: interrupt request
>> + * @scrq_instance: async subq
>> + *
>> + **/
>> +static irqreturn_t ibmvfc_interrupt_asyncq(int irq, void *scrq_instance)
>> +{
>> + struct ibmvfc_queue *scrq = (struct ibmvfc_queue *)scrq_instance;
>> +
>> + ENTER;
>> +
>> + ibmvfc_toggle_scrq_irq(scrq, 0);
>> + ibmvfc_drain_async_subq(scrq);
>> +
>> + LEAVE;
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> static void ibmvfc_drain_sub_crq(struct ibmvfc_queue *scrq)
>> {
>> struct ibmvfc_crq *crq;
>> @@ -5500,6 +5670,8 @@ static void ibmvfc_npiv_login_done(struct ibmvfc_event
>> *evt)
>> unsigned int npiv_max_sectors;
>> int level = IBMVFC_DEFAULT_LOG_LEVEL;
>>
>> + ENTER;
>> +
>> switch (mad_status) {
>> case IBMVFC_MAD_SUCCESS:
>> ibmvfc_free_event(evt);
>> @@ -5578,6 +5750,8 @@ static void ibmvfc_npiv_login_done(struct ibmvfc_event
>> *evt)
>> ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_QUERY);
>> wake_up(&vhost->work_wait_q);
>> }
>> +
>> + LEAVE;
>
> Also, please drop the ENTER/LEAVE macros as these look like debug artifacts.
> Especially, in common code paths like the interrupt handlers.
Done.
-Dave
