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