| Message ID | 20190905030845.15540-2-oohall@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/5] xive, interrupts: Add a mask() source op | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (7b12d5489fcfd73ef7ec0cb27eff7f8a5f13b238) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On 05/09/2019 05:08, Oliver O'Halloran wrote: > The array is currently a static constant inside psi_p9_irq_name() which > returns a malloc()ed pointer due to the API required by the interrupt > source ops interface. Make the name array a static global and move it > further up in the file so we can use it in other functions. > > Signed-off-by: Oliver O'Halloran <oohall@gmail.com> one related comment below. Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > hw/psi.c | 54 +++++++++++++++++++++++++++--------------------------- > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/hw/psi.c b/hw/psi.c > index a74c105ff0ec..99ec06ac488a 100644 > --- a/hw/psi.c > +++ b/hw/psi.c > @@ -509,6 +509,33 @@ static const struct irq_source_ops psi_p8_irq_ops = { > .name = psi_p8_irq_name, > }; > > +static const char *p9_psi_int_names[P9_PSI_NUM_IRQS] = { > + "psi:fsp", > + "psi:occ", > + "psi:fsi", > + "psi:lpchc", > + "psi:local_err", > + "psi:global_err", > + "psi:external", > + "psi:lpc_serirq_mux0", /* Have a callback to get name ? */ > + "psi:lpc_serirq_mux1", /* Have a callback to get name ? */ > + "psi:lpc_serirq_mux2", /* Have a callback to get name ? */ > + "psi:lpc_serirq_mux3", /* Have a callback to get name ? */ > + "psi:i2c", > + "psi:dio", > + "psi:psu" > +}; > + > +static char *psi_p9_irq_name(struct irq_source *is, uint32_t isn) > +{ > + struct psi *psi = is->data; > + uint32_t idx = isn - psi->interrupt; > + > + if (idx >= ARRAY_SIZE(p9_psi_int_names)) > + return NULL; > + return strdup(p9_psi_int_names[idx]); I haven't seen any of the interrupt sources allocating the name returned. I think we could remove the strdup from all name() handlers and the free() from add_opal_interrupts() one day. > +} > + > static void psihb_p9_interrupt(struct irq_source *is, uint32_t isn) > { > struct psi *psi = is->data; > @@ -583,33 +610,6 @@ static uint64_t psi_p9_irq_attributes(struct irq_source *is __unused, > return IRQ_ATTR_TARGET_OPAL | IRQ_ATTR_TYPE_LSI; > } > > -static char *psi_p9_irq_name(struct irq_source *is, uint32_t isn) > -{ > - struct psi *psi = is->data; > - uint32_t idx = isn - psi->interrupt; > - > - static const char *names[P9_PSI_NUM_IRQS] = { > - "psi:fsp", > - "psi:occ", > - "psi:fsi", > - "psi:lpchc", > - "psi:local_err", > - "psi:global_err", > - "psi:external", > - "psi:lpc_serirq_mux0", /* Have a callback to get name ? */ > - "psi:lpc_serirq_mux1", /* Have a callback to get name ? */ > - "psi:lpc_serirq_mux2", /* Have a callback to get name ? */ > - "psi:lpc_serirq_mux3", /* Have a callback to get name ? */ > - "psi:i2c", > - "psi:dio", > - "psi:psu" > - }; > - > - if (idx >= P9_PSI_NUM_IRQS) > - return NULL; > - return strdup(names[idx]); > -} > - > static const struct irq_source_ops psi_p9_irq_ops = { > .interrupt = psihb_p9_interrupt, > .attributes = psi_p9_irq_attributes, >
On Thu, Sep 5, 2019 at 5:02 PM Cédric Le Goater <clg@kaod.org> wrote: > > > +static char *psi_p9_irq_name(struct irq_source *is, uint32_t isn) > > +{ > > + struct psi *psi = is->data; > > + uint32_t idx = isn - psi->interrupt; > > + > > + if (idx >= ARRAY_SIZE(p9_psi_int_names)) > > + return NULL; > > + return strdup(p9_psi_int_names[idx]); > > I haven't seen any of the interrupt sources allocating the name returned. > > I think we could remove the strdup from all name() handlers and the free() > from add_opal_interrupts() one day. Yeah looks like they don't currently. I think the idea was to return a malloc()ed string so the source could label the interrupts them based on the source name. It's not needed for the PSI since there's only one and you can use the HWIRQ number to work out what chip it's from, but for the PHBs it's good to be able to differentiate since otherwise you have a sea of interrupts labelled "PHB Error". Granted we don't have the PHB error interrupts wired up at the moment, but it's something I'll get around to eventually. I suppose we could just generate them before registering the source. Oliver
On 05/09/2019 09:46, Oliver O'Halloran wrote: > On Thu, Sep 5, 2019 at 5:02 PM Cédric Le Goater <clg@kaod.org> wrote: >> >>> +static char *psi_p9_irq_name(struct irq_source *is, uint32_t isn) >>> +{ >>> + struct psi *psi = is->data; >>> + uint32_t idx = isn - psi->interrupt; >>> + >>> + if (idx >= ARRAY_SIZE(p9_psi_int_names)) >>> + return NULL; >>> + return strdup(p9_psi_int_names[idx]); >> >> I haven't seen any of the interrupt sources allocating the name returned. >> >> I think we could remove the strdup from all name() handlers and the free() >> from add_opal_interrupts() one day. > > Yeah looks like they don't currently. I think the idea was to return a > malloc()ed string so the source could label the interrupts them based > on the source name. It's not needed for the PSI since there's only one You see both PSI on a boston : IRQ HWIRQ NAME 496: 000ffff0 opal-psi:fsp 497: 000ffff1 opal-psi:occ 498: 000ffff2 opal-psi:fsi 499: 000ffff3 opal-psi:lpchc 500: 000ffff4 opal-psi:local_err 501: 000ffff5 opal-psi:global_err 502: 000ffff6 opal-psi:external 503: 000ffff7 opal-psi:lpc_serirq_mux0 504: 000ffff8 opal-psi:lpc_serirq_mux1 505: 000ffff9 opal-psi:lpc_serirq_mux2 506: 000ffffa opal-psi:lpc_serirq_mux3 507: 000ffffb opal-psi:i2c 508: 000ffffc opal-psi:dio 509: 000ffffd opal-psi:psu 510: 010ffff0 opal-psi:fsp 511: 010ffff1 opal-psi:occ 512: 010ffff2 opal-psi:fsi 55: 010ffff3 opal-psi:lpchc 56: 010ffff4 opal-psi:local_err 57: 010ffff5 opal-psi:global_err 58: 010ffff6 opal-psi:external 63: 010ffffb opal-psi:i2c 64: 010ffffc opal-psi:dio 65: 010ffffd opal-psi:psu > and you can use the HWIRQ number to work out what chip it's from, yes. This is true for any HWIRQ number. The XIVE block id is embedded in the IRQ number (see above) and it gives you some information on the chip id. > but > for the PHBs it's good to be able to differentiate since otherwise you > have a sea of interrupts labelled "PHB Error". Granted we don't have > the PHB error interrupts wired up at the moment, but it's something > I'll get around to eventually. OK. I understand that the goal was to include the chip id in the literal name. It is a good idea to keep the strdup but we should add the required snprintf() then. C. > I suppose we could just generate them before registering the source. > > Oliver >
On Thu, Sep 5, 2019 at 6:06 PM Cédric Le Goater <clg@kaod.org> wrote: > > On 05/09/2019 09:46, Oliver O'Halloran wrote: > > On Thu, Sep 5, 2019 at 5:02 PM Cédric Le Goater <clg@kaod.org> wrote: > >> > >>> +static char *psi_p9_irq_name(struct irq_source *is, uint32_t isn) > >>> +{ > >>> + struct psi *psi = is->data; > >>> + uint32_t idx = isn - psi->interrupt; > >>> + > >>> + if (idx >= ARRAY_SIZE(p9_psi_int_names)) > >>> + return NULL; > >>> + return strdup(p9_psi_int_names[idx]); > >> > >> I haven't seen any of the interrupt sources allocating the name returned. > >> > >> I think we could remove the strdup from all name() handlers and the free() > >> from add_opal_interrupts() one day. > > > > Yeah looks like they don't currently. I think the idea was to return a > > malloc()ed string so the source could label the interrupts them based > > on the source name. It's not needed for the PSI since there's only one > > You see both PSI on a boston : What I meant was there's one PSI per chip. The fact we've got two sets of interrupts isn't a problem because the HWIRQ allows you to differentiate. >*snip* > > > and you can use the HWIRQ number to work out what chip it's from, > > yes. This is true for any HWIRQ number. The XIVE block id is embedded in > the IRQ number (see above) and it gives you some information on the chip id. Yes, but it only tells you the chip ID. When there's multiple units on a chip (PHBs, or NPUs on swift) you need to encode something into the name. > > but > > for the PHBs it's good to be able to differentiate since otherwise you > > have a sea of interrupts labelled "PHB Error". Granted we don't have > > the PHB error interrupts wired up at the moment, but it's something > > I'll get around to eventually. > > OK. I understand that the goal was to include the chip id in the literal > name. It is a good idea to keep the strdup but we should add the required > snprintf() then. ok Oliver
>> yes. This is true for any HWIRQ number. The XIVE block id is embedded in >> the IRQ number (see above) and it gives you some information on the chip id. > > Yes, but it only tells you the chip ID. When there's multiple units on > a chip (PHBs, or NPUs on swift) you need to encode something into the > name. There are predefined ranges for each PHB. So you can tell for these. It's more difficult for the NPU interrupts because they are in the IPI set. Anyhow it is good to have a literal name for all. Thanks, C.
diff --git a/hw/psi.c b/hw/psi.c index a74c105ff0ec..99ec06ac488a 100644 --- a/hw/psi.c +++ b/hw/psi.c @@ -509,6 +509,33 @@ static const struct irq_source_ops psi_p8_irq_ops = { .name = psi_p8_irq_name, }; +static const char *p9_psi_int_names[P9_PSI_NUM_IRQS] = { + "psi:fsp", + "psi:occ", + "psi:fsi", + "psi:lpchc", + "psi:local_err", + "psi:global_err", + "psi:external", + "psi:lpc_serirq_mux0", /* Have a callback to get name ? */ + "psi:lpc_serirq_mux1", /* Have a callback to get name ? */ + "psi:lpc_serirq_mux2", /* Have a callback to get name ? */ + "psi:lpc_serirq_mux3", /* Have a callback to get name ? */ + "psi:i2c", + "psi:dio", + "psi:psu" +}; + +static char *psi_p9_irq_name(struct irq_source *is, uint32_t isn) +{ + struct psi *psi = is->data; + uint32_t idx = isn - psi->interrupt; + + if (idx >= ARRAY_SIZE(p9_psi_int_names)) + return NULL; + return strdup(p9_psi_int_names[idx]); +} + static void psihb_p9_interrupt(struct irq_source *is, uint32_t isn) { struct psi *psi = is->data; @@ -583,33 +610,6 @@ static uint64_t psi_p9_irq_attributes(struct irq_source *is __unused, return IRQ_ATTR_TARGET_OPAL | IRQ_ATTR_TYPE_LSI; } -static char *psi_p9_irq_name(struct irq_source *is, uint32_t isn) -{ - struct psi *psi = is->data; - uint32_t idx = isn - psi->interrupt; - - static const char *names[P9_PSI_NUM_IRQS] = { - "psi:fsp", - "psi:occ", - "psi:fsi", - "psi:lpchc", - "psi:local_err", - "psi:global_err", - "psi:external", - "psi:lpc_serirq_mux0", /* Have a callback to get name ? */ - "psi:lpc_serirq_mux1", /* Have a callback to get name ? */ - "psi:lpc_serirq_mux2", /* Have a callback to get name ? */ - "psi:lpc_serirq_mux3", /* Have a callback to get name ? */ - "psi:i2c", - "psi:dio", - "psi:psu" - }; - - if (idx >= P9_PSI_NUM_IRQS) - return NULL; - return strdup(names[idx]); -} - static const struct irq_source_ops psi_p9_irq_ops = { .interrupt = psihb_p9_interrupt, .attributes = psi_p9_irq_attributes,
The array is currently a static constant inside psi_p9_irq_name() which returns a malloc()ed pointer due to the API required by the interrupt source ops interface. Make the name array a static global and move it further up in the file so we can use it in other functions. Signed-off-by: Oliver O'Halloran <oohall@gmail.com> --- hw/psi.c | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-)