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

[Marc Zyngier <maz@kernel.org>]

On 2020-09-14 15:57, Grzegorz Jaszczyk wrote:
> On Sat, 12 Sep 2020 at 11:44, Marc Zyngier <maz@kernel.org> 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...