Re: [PATCH net-next 3/6] net: ipa: enable IPA interrupt handlers separate from registration

[Alex Elder]

On 12/31/22 11:56 AM, Caleb Connolly wrote:
> > diff --git a/drivers/net/ipa/ipa_interrupt.h 
> > b/drivers/net/ipa/ipa_interrupt.h
> > index f31fd9965fdc6..5f7d2e90ea337 100644
> > --- a/drivers/net/ipa/ipa_interrupt.h
> > +++ b/drivers/net/ipa/ipa_interrupt.h
> > @@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct 
> > ipa_interrupt *interrupt);
> >    */
> >   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
> > +/**
> > + * ipa_interrupt_enable() - Enable an IPA interrupt type
> > + * @ipa:    IPA pointer
> > + * @ipa_irq:    IPA interrupt ID
> > + */
> > +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
>
> I think you forgot a forward declaration for enum ipa_irq_id
>
> Kind Regards,
> Caleb

OK I checked this.

You are correct that ipa_irq_id should be declared as an
enum at the top of "ipa_interrupt.h".  In addition to the
new function declarations, there were some existing
references to the enumerated type.  I believe this became
a (not-reported) problem starting with this commit:

  322053105f095 net: ipa: move definition of enum ipa_irq_id

It being missing did not result in any build warnings,
however.  Here's why.

The ipa_irq_id enumerated type is defined in "ipa_reg.h".

Note that "ipa_reg.h" is included by "ipa_endpoint.h".  So if
"ipa_endpoint.h" is included before "ipa_interrupt.h", the
ipa_irq_id type will have been defined before it's referenced
in "ipa_interrupt.h".

These files include "ipa_interrupt.h":

ipa.h:
It is included after "ipa_endpoint.h", so the enumerated type
is defined before it's needed in "ipa_interrupt.h".

ipa_main.c:
Here too, "ipa_endpoint.h" (as well as "ipa_reg.h") is included
before "ipa_interrupt.h", so the enumerated type is defined
before it's used there.

ipa_interrupt.c
In this case "ipa_reg.h" is included, then "ipa_endpoint.h",
before "ipa_interrupt.h".  So again, the enumerated type is
defined before it's referenced in "ipa_interrupt.h".

Nevertheless, your point is a good one and I'm going to
implement your suggestion when I post version 2.

Thank you!

					-Alex