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

From: Marc Zyngier <maz@kernel.org>
To: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Cc: tglx@linutronix.de, jason@lakedaemon.net, "Anna,
	Suman" <s-anna@ti.com>,
	robh+dt@kernel.org, Lee Jones <lee.jones@linaro.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	David Lechner <david@lechnology.com>,
	"Bajjuri, Praneeth" <praneeth@ti.com>,
	"Andrew F . Davis" <afd@ti.com>, Roger Quadros <rogerq@ti.com>
Subject: Re: [RESEND PATCH v5 2/5] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
Date: Mon, 14 Sep 2020 16:20:49 +0100	[thread overview]
Message-ID: <98edb75abcd08e93b46c7b6dcf26ad23@kernel.org> (raw)
In-Reply-To: <CAMxfBF6Qj=_uVA5t2mEkncRQ3jb+ZZZtc0=qvw7YsZzxCw-UfQ@mail.gmail.com>

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...