Re: [PATCH v2 06/13] ACPI: resources: Add wake_capable parameter to acpi_dev_irq_flags

From: Raul E Rangel
Date: Wed Sep 14 2022 - 16:49:59 EST

On Tue, Sep 13, 2022 at 08:21:15PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 12, 2022 at 04:13:10PM -0600, Raul E Rangel wrote:
> > ACPI IRQ/Interrupt resources contain a bit that describes if the
> > interrupt should wake the system. This change exposes that bit via
> > a new IORESOURCE_IRQ_WAKECAPABLE flag. Drivers should check this flag
> > before arming an IRQ to wake the system.
>
> ...
>
> > static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
> > u32 hwirq, u8 triggering,
> > u8 polarity, u8 shareable,
> > + u8 wake_capable,
> > struct acpi_irq_parse_one_ctx *ctx)
>
> This function is used only in scope of a single C-file. Why instead not
> converting it to use some internal structure and acpi_irq_parse_one_cb()
> becomes like:
>
> struct internal_struct s;
>
> switch (ares->type) {
> case ACPI_RESOURCE_TYPE_IRQ:
> ...fill internal_struct...
> break;
> case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> ...fill internal_struct...
> break;
> default:
> return AE_OK;
>
> acpi_irq_parse_one_match(&s);
> return AE_CTRL_TERMINATE;
>
> ?
>
> ...
>
> > + acpi_dev_get_irqresource(res,
> > + ext_irq->interrupts[index],
> > + ext_irq->triggering,
> > + ext_irq->polarity,
> > + ext_irq->shareable,
> > + ext_irq->wake_capable, false);
>
> Ditto.
>
> Actually it can be shared structure for these too.
>

I tried your suggestion, but I honestly think it hurts readability. It's
also a little scary because the compiler doesn't guarantee all the
members of the struct are filled out, unlike having the explicit
parameters. Here is the patch:

diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c
index 5483cf9a28e3a0..8549ccefa5d03c 100644
--- a/drivers/acpi/irq.c
+++ b/drivers/acpi/irq.c
@@ -139,35 +139,37 @@ struct acpi_irq_parse_one_ctx {
struct irq_fwspec *fwspec;
};

+struct acpi_irq_parse_one_match_params {
+ struct fwnode_handle *fwnode;
+ u32 hwirq;
+ u8 triggering;
+ u8 polarity;
+ u8 shareable;
+ u8 wake_capable;
+}
+
/**
- * acpi_irq_parse_one_match - Handle a matching IRQ resource.
- * @fwnode: matching fwnode
- * @hwirq: hardware IRQ number
- * @triggering: triggering attributes of hwirq
- * @polarity: polarity attributes of hwirq
- * @polarity: polarity attributes of hwirq
- * @shareable: shareable attributes of hwirq
- * @wake_capable: wake capable attribute of hwirq
+ * acpi_irq_parse_update_ctx - Handle a matching IRQ resource.
+ * @params: IRQ parameters
* @ctx: acpi_irq_parse_one_ctx updated by this function
*
* Description:
- * Handle a matching IRQ resource by populating the given ctx with
- * the information passed.
+ * Update the given ctx with the IRQ information passed.
*/
-static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
- u32 hwirq, u8 triggering,
- u8 polarity, u8 shareable,
- u8 wake_capable,
- struct acpi_irq_parse_one_ctx *ctx)
+static inline void
+acpi_irq_parse_update_ctx(const struct parse_one_match *params,
+ struct acpi_irq_parse_one_ctx *ctx)
{
- if (!fwnode)
+ if (!params->fwnode)
return;
ctx->rc = 0;
- *ctx->res_flags = acpi_dev_irq_flags(triggering, polarity, shareable,
- wake_capable);
- ctx->fwspec->fwnode = fwnode;
- ctx->fwspec->param[0] = hwirq;
- ctx->fwspec->param[1] = acpi_dev_get_irq_type(triggering, polarity);
+ *ctx->res_flags =
+ acpi_dev_irq_flags(params->triggering, params->polarity,
+ params->shareable, params->wake_capable);
+ ctx->fwspec->fwnode = params->fwnode;
+ ctx->fwspec->param[0] = params->hwirq;
+ ctx->fwspec->param[1] =
+ acpi_dev_get_irq_type(params->triggering, params->polarity);
ctx->fwspec->param_count = 2;
}

@@ -182,7 +184,7 @@ static inline void acpi_irq_parse_one_match(struct fwnode_handle *fwnode,
* might contain multiple interrupts we check if the index is within this
* one's interrupt array, otherwise we subtract the current resource IRQ
* count from the lookup index to prepare for the next resource.
- * Once a match is found we call acpi_irq_parse_one_match to populate
+ * Once a match is found we call acpi_irq_parse_update_ctx to populate
* the result and end the walk by returning AE_CTRL_TERMINATE.
*
* Return:
@@ -195,7 +197,7 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
struct acpi_irq_parse_one_ctx *ctx = context;
struct acpi_resource_irq *irq;
struct acpi_resource_extended_irq *eirq;
- struct fwnode_handle *fwnode;
+ struct acpi_irq_parse_one_match_params params = {0};

switch (ares->type) {
case ACPI_RESOURCE_TYPE_IRQ:
@@ -204,11 +206,13 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
ctx->index -= irq->interrupt_count;
return AE_OK;
}
- fwnode = acpi_get_gsi_domain_id(irq->interrupts[ctx->index]);
- acpi_irq_parse_one_match(fwnode, irq->interrupts[ctx->index],
- irq->triggering, irq->polarity,
- irq->shareable, irq->wake_capable,
- ctx);
+ params.fwnode =
+ acpi_get_gsi_domain_id(irq->interrupts[ctx->index]);
+ params.hwirq = irq->interrupts[ctx->index];
+ params.triggering = irq->triggering;
+ params.polarity = irq->polarity;
+ params.shareable = irq->shareable;
+ params.wake_capable = irq->wake_capable;
return AE_CTRL_TERMINATE;
case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
eirq = &ares->data.extended_irq;
@@ -218,16 +222,22 @@ static acpi_status acpi_irq_parse_one_cb(struct acpi_resource *ares,
ctx->index -= eirq->interrupt_count;
return AE_OK;
}
- fwnode = acpi_get_irq_source_fwhandle(&eirq->resource_source,
- eirq->interrupts[ctx->index]);
- acpi_irq_parse_one_match(fwnode, eirq->interrupts[ctx->index],
- eirq->triggering, eirq->polarity,
- eirq->shareable, eirq->wake_capable,
- ctx);
- return AE_CTRL_TERMINATE;
+ params.fwnode = acpi_get_irq_source_fwhandle(
+ &eirq->resource_source, eirq->interrupts[ctx->index]);
+ params.hwirq = eirq->interrupts[ctx->index];
+ params.triggering = eirq->triggering;
+ params.polarity = eirq->polarity;
+ params.shareable = eirq->shareable;
+ params.wake_capable = eirq->wake_capable;
+
+ default:
+ return AE_OK;
}

- return AE_OK;
+ acpi_irq_parse_update_ctx(&params, ctx);
+
+ return AE_CTRL_TERMINATE;
+
}

/**