On 4/15/2025 2:05 AM, Meghana Malladi wrote:
> The ICSS IEP driver tracks perout and pps enable state with flags.
> Currently when disabling pps and perout signals during icss_iep_exit(),
> results in NULL pointer dereference for perout.
>
> To fix the null pointer dereference issue, the icss_iep_perout_enable_hw
> function can be modified to directly clear the IEP CMP registers when
> disabling PPS or PEROUT, without referencing the ptp_perout_request
> structure, as its contents are irrelevant in this case.
>
> Fixes: 9b115361248d ("net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init")
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/7b1c7c36-363a-4085-b26c-4f210bee1df6@stanley.mountain/
> Signed-off-by: Meghana Malladi <m-malladi@xxxxxx>
> ---
>
Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
> Changes from v3 (v4-v3):
> - Fix the logic in icss_iep_perout_enable_hw() to clear IEP registers
> when disabling periodic signal
>
> drivers/net/ethernet/ti/icssg/icss_iep.c | 121 +++++++++++------------
> 1 file changed, 58 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index b4a34c57b7b4..2a1c43316f46 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -430,64 +446,39 @@ static int icss_iep_perout_enable_hw(struct icss_iep *iep,
> if (ret)
> return ret;
>
> - if (on) {
> - /* Configure CMP */
> - regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp));
> - if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT)
> - regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp));
> - /* Configure SYNC, based on req on width */
> - regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG,
> - div_u64(ns_width, iep->def_inc));
> - regmap_write(iep->map, ICSS_IEP_SYNC0_PERIOD_REG, 0);
> - regmap_write(iep->map, ICSS_IEP_SYNC_START_REG,
> - div_u64(ns_start, iep->def_inc));
> - regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG, 0); /* one-shot mode */
> - /* Enable CMP 1 */
> - regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
> - IEP_CMP_CFG_CMP_EN(1), IEP_CMP_CFG_CMP_EN(1));
> - } else {
> - /* Disable CMP 1 */
> - regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
> - IEP_CMP_CFG_CMP_EN(1), 0);
> -
> - /* clear regs */
> - regmap_write(iep->map, ICSS_IEP_CMP1_REG0, 0);
> - if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT)
> - regmap_write(iep->map, ICSS_IEP_CMP1_REG1, 0);
> - }
> + /* Configure CMP */
> + regmap_write(iep->map, ICSS_IEP_CMP1_REG0, lower_32_bits(cmp));
> + if (iep->plat_data->flags & ICSS_IEP_64BIT_COUNTER_SUPPORT)
> + regmap_write(iep->map, ICSS_IEP_CMP1_REG1, upper_32_bits(cmp));
> + /* Configure SYNC, based on req on width */
> + regmap_write(iep->map, ICSS_IEP_SYNC_PWIDTH_REG,
> + div_u64(ns_width, iep->def_inc));
> + regmap_write(iep->map, ICSS_IEP_SYNC0_PERIOD_REG, 0);
> + regmap_write(iep->map, ICSS_IEP_SYNC_START_REG,
> + div_u64(ns_start, iep->def_inc));
> + regmap_write(iep->map, ICSS_IEP_SYNC_CTRL_REG, 0); /* one-shot mode */
> + /* Enable CMP 1 */
> + regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
> + IEP_CMP_CFG_CMP_EN(1), IEP_CMP_CFG_CMP_EN(1));
Nice to see this also has a marked improvement with removing a level of
indentation.
> @@ -498,11 +489,21 @@ static int icss_iep_perout_enable(struct icss_iep *iep,
> {
> int ret = 0;
>
> + if (!on)
> + goto disable;
> +
> /* Reject requests with unsupported flags */
> if (req->flags & ~(PTP_PEROUT_DUTY_CYCLE |
> PTP_PEROUT_PHASE))
> return -EOPNOTSUPP;
>
This likely causes a textual conflict with my .supported_perout_flags
patch. It looks like it wouldn't be too difficult to resolve though.
> + /* Set default "on" time (1ms) for the signal if not passed by the app */
> + if (!(req->flags & PTP_PEROUT_DUTY_CYCLE)) {
> + req->on.sec = 0;
> + req->on.nsec = NSEC_PER_MSEC;
> + }
> +
Regards,
Jake
