All of lore.kernel.org
 help / color / mirror / Atom feed
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...

WARNING: multiple messages have this Message-ID
From: Marc Zyngier <maz@kernel.org>
To: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Cc: devicetree@vger.kernel.org, Roger Quadros <rogerq@ti.com>,
	linux-omap@vger.kernel.org, jason@lakedaemon.net, "Bajjuri,
	Praneeth" <praneeth@ti.com>,
	linux-kernel@vger.kernel.org, "Andrew F . Davis" <afd@ti.com>,
	robh+dt@kernel.org, tglx@linutronix.de,
	Lee Jones <lee.jones@linaro.org>,
	linux-arm-kernel@lists.infradead.org,
	David Lechner <david@lechnology.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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-09-14 15:21 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-31 15:09 [RESEND PATCH v5 0/5] Add TI PRUSS Local Interrupt Controller IRQChip driver Grzegorz Jaszczyk
2020-08-31 15:09 ` Grzegorz Jaszczyk
2020-08-31 15:09 ` [RESEND PATCH v5 1/5] dt-bindings: irqchip: Add PRU-ICSS interrupt controller bindings Grzegorz Jaszczyk
2020-08-31 15:09   ` Grzegorz Jaszczyk
2020-08-31 15:09 ` [RESEND PATCH v5 2/5] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts Grzegorz Jaszczyk
2020-08-31 15:09   ` Grzegorz Jaszczyk
2020-08-31 15:42   ` Randy Dunlap
2020-08-31 15:42     ` Randy Dunlap
2020-09-12  9:44   ` Marc Zyngier
2020-09-12  9:44     ` Marc Zyngier
2020-09-14 14:57     ` Grzegorz Jaszczyk
2020-09-14 14:57       ` Grzegorz Jaszczyk
2020-09-14 15:20       ` Marc Zyngier [this message]
2020-09-14 15:20         ` Marc Zyngier
2020-08-31 15:09 ` [RESEND PATCH v5 3/5] irqchip/irq-pruss-intc: Add logic for handling reserved interrupts Grzegorz Jaszczyk
2020-08-31 15:09   ` Grzegorz Jaszczyk
2020-08-31 15:09 ` [RESEND PATCH v5 4/5] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops Grzegorz Jaszczyk
2020-08-31 15:09   ` [RESEND PATCH v5 4/5] irqchip/irq-pruss-intc: Implement irq_{get, set}_irqchip_state ops Grzegorz Jaszczyk
2020-09-12  9:49   ` [RESEND PATCH v5 4/5] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops Marc Zyngier
2020-09-12  9:49     ` [RESEND PATCH v5 4/5] irqchip/irq-pruss-intc: Implement irq_{get, set}_irqchip_state ops Marc Zyngier
2020-09-14 14:59     ` [RESEND PATCH v5 4/5] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops Grzegorz Jaszczyk
2020-09-14 14:59       ` Grzegorz Jaszczyk
2020-08-31 15:09 ` [RESEND PATCH v5 5/5] irqchip/irq-pruss-intc: Add support for ICSSG INTC on K3 SoCs Grzegorz Jaszczyk
2020-08-31 15:09   ` Grzegorz Jaszczyk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98edb75abcd08e93b46c7b6dcf26ad23@kernel.org \
    --to=maz@kernel.org \
    --cc=afd@ti.com \
    --cc=david@lechnology.com \
    --cc=devicetree@vger.kernel.org \
    --cc=grzegorz.jaszczyk@linaro.org \
    --cc=jason@lakedaemon.net \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=praneeth@ti.com \
    --cc=robh+dt@kernel.org \
    --cc=rogerq@ti.com \
    --cc=s-anna@ti.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.