Re: [RESEND PATCH v5 2/5] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts

[Marc Zyngier]

On 2020-09-14 15:57, Grzegorz Jaszczyk wrote:
> On Sat, 12 Sep 2020 at 11:44, Marc Zyngier <maz@xxxxxxxxxx> wrote:

[...]

> > > +static void pruss_intc_update_cmr(struct pruss_intc *intc, int evt, s8 ch)
> > > +{
> > > +     u32 idx, offset, val;
> > > +
> > > +     idx = evt / CMR_EVT_PER_REG;
> > > +     offset = (evt % CMR_EVT_PER_REG) * CMR_EVT_MAP_BITS;
> > > +
> > > +     val = pruss_intc_read_reg(intc, PRU_INTC_CMR(idx));
> > > +     val &= ~(CMR_EVT_MAP_MASK << offset);
> > > +     val |= ch << offset;
> >
> > Why is 'ch' a signed value? Shifting a signed value, specially when
> > casing it to a larger, unsigned type definitely is in UB territory.
> > Similar funnies can be said about evt.
> >
> > And given that the caller does use unsigned types, you really are
> > asking for trouble. Please fix this.
>
> Sure, thank you for pointing this out - I will change to u8.

Please change evt too. Anything that is used to compute masks/shifts
should be unsigned.

[...]

> > > +static void pruss_intc_init(struct pruss_intc *intc)
> > > +{
> > > +     const struct pruss_intc_match_data *soc_config = intc->soc_config;
> > > +     int i;
> > > +     int num_chnl_map_regs = DIV_ROUND_UP(soc_config->num_system_events,
> > > +                                          CMR_EVT_PER_REG);
> > > +     int num_host_intr_regs = DIV_ROUND_UP(soc_config->num_host_events,
> > > +                                           HMR_CH_PER_REG);
> > > +     int num_event_type_regs =
> > > +                     DIV_ROUND_UP(soc_config->num_system_events, 32);
> >
> > Please keep assignments on a single line.
>
> Keeping entire assignment in single line will break the 80 columns
> rule, what about changing it into:

There is no such thing as a "80 column rule". As I often say, I no
longer own a vt100.

> -       int i;
> -       int num_chnl_map_regs = 
> DIV_ROUND_UP(soc_config->num_system_events,
> -                                            CMR_EVT_PER_REG);
> -       int num_host_intr_regs = 
> DIV_ROUND_UP(soc_config->num_host_events,
> -                                             HMR_CH_PER_REG);
> -       int num_event_type_regs =
> -                       DIV_ROUND_UP(soc_config->num_system_events, 
> 32);
> +       int num_chnl_map_regs, num_host_intr_regs, num_event_type_regs, 
> i;
> +
> +       num_chnl_map_regs = DIV_ROUND_UP(soc_config->num_system_events,
> +                                        CMR_EVT_PER_REG);
> +       num_host_intr_regs = DIV_ROUND_UP(soc_config->num_host_events,
> +                                         HMR_CH_PER_REG);
> +       num_event_type_regs = 
> DIV_ROUND_UP(soc_config->num_system_events, 32);
>
> Will it be ok?

Sure.

        M.
--
Jazz is not dead. It just smells funny...