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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 14 Sep 2020 16:20:49 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Grzegorz Jaszczyk <grzegorz.jaszczyk@...aro.org>
Cc:     tglx@...utronix.de, jason@...edaemon.net,
        "Anna, Suman" <s-anna@...com>, robh+dt@...nel.org,
        Lee Jones <lee.jones@...aro.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        David Lechner <david@...hnology.com>,
        "Bajjuri, Praneeth" <praneeth@...com>,
        "Andrew F . Davis" <afd@...com>, Roger Quadros <rogerq@...com>
Subject: Re: [RESEND PATCH v5 2/5] irqchip/irq-pruss-intc: Add a PRUSS irqchip
 driver for PRUSS interrupts

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