| Message ID | 20240618104310.1429515-4-karol.kolacinski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: Fix incorrect input/output pin behavior | expand |
On Tue, Jun 18, 2024 at 12:41:38PM +0200, Karol Kolacinski wrote: > From: Jacob Keller <jacob.e.keller@intel.com> > > The driver receives requests for configuring pins via the .enable > callback of the PTP clock object. These requests come into the driver > with flags which modify the requested behavior from userspace. Current > implementation in ice does not reject flags that it doesn't support. > This causes the driver to incorrectly apply requests with such flags as > PTP_PEROUT_DUTY_CYCLE, or any future flags added by the kernel which it > is not yet aware of. > > Fix this by properly validating flags in both ice_ptp_cfg_perout and > ice_ptp_cfg_extts. Ensure that we check by bit-wise negating supported > flags rather than just checking and rejecting known un-supported flags. > This is preferable, as it ensures better compatibility with future > kernels. > > Fixes: 172db5f91d5f ("ice: add support for auxiliary input/output pins") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> Hi Jacob and Karol, Some minor feedback from my side. ... > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c > index b952cad42f92..5fa377786f4c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -1591,14 +1591,23 @@ void ice_ptp_extts_event(struct ice_pf *pf) > * @store: If set to true, the values will be stored > * > * Configure an external timestamp event on the requested channel. > - */ > -static void ice_ptp_cfg_extts(struct ice_pf *pf, unsigned int chan, > - struct ice_extts_channel *config, bool store) > + * > + * Return: 0 on sucess, -EOPNOTUSPP on unsupported flags nit: success Flagged by checkpatch.pl --codespell > + */ > +static int ice_ptp_cfg_extts(struct ice_pf *pf, unsigned int chan, > + struct ice_extts_channel *config, bool store) > { > u32 func, aux_reg, gpio_reg, irq_reg; > struct ice_hw *hw = &pf->hw; > u8 tmr_idx; > > + /* Reject requests with unsupported flags */ > + if (config->flags & ~(PTP_ENABLE_FEATURE | > + PTP_RISING_EDGE | > + PTP_FALLING_EDGE | > + PTP_STRICT_FLAGS)) > + return -EOPNOTSUPP; The line above should to be indented one more tab. Clearly this makes no difference at run-time, but it takes a while (for me) to parse things as-is. ... > @@ -1697,6 +1708,9 @@ static int ice_ptp_cfg_clkout(struct ice_pf *pf, unsigned int chan, > u32 func, val, gpio_pin; > u8 tmr_idx; > > + if (config->flags & ~PTP_PEROUT_PHASE) > + return -EOPNOTSUPP; A little further down in this function it is assumed that config may be NULL. So I think the code above needs to be updated to take that into account too. Flagged by Smatch. ...
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index b952cad42f92..5fa377786f4c 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -1591,14 +1591,23 @@ void ice_ptp_extts_event(struct ice_pf *pf) * @store: If set to true, the values will be stored * * Configure an external timestamp event on the requested channel. - */ -static void ice_ptp_cfg_extts(struct ice_pf *pf, unsigned int chan, - struct ice_extts_channel *config, bool store) + * + * Return: 0 on sucess, -EOPNOTUSPP on unsupported flags + */ +static int ice_ptp_cfg_extts(struct ice_pf *pf, unsigned int chan, + struct ice_extts_channel *config, bool store) { u32 func, aux_reg, gpio_reg, irq_reg; struct ice_hw *hw = &pf->hw; u8 tmr_idx; + /* Reject requests with unsupported flags */ + if (config->flags & ~(PTP_ENABLE_FEATURE | + PTP_RISING_EDGE | + PTP_FALLING_EDGE | + PTP_STRICT_FLAGS)) + return -EOPNOTSUPP; + tmr_idx = hw->func_caps.ts_func_info.tmr_index_owned; irq_reg = rd32(hw, PFINT_OICR_ENA); @@ -1639,6 +1648,8 @@ static void ice_ptp_cfg_extts(struct ice_pf *pf, unsigned int chan, if (store) memcpy(&pf->ptp.extts_channels[chan], config, sizeof(*config)); + + return 0; } /** @@ -1697,6 +1708,9 @@ static int ice_ptp_cfg_clkout(struct ice_pf *pf, unsigned int chan, u32 func, val, gpio_pin; u8 tmr_idx; + if (config->flags & ~PTP_PEROUT_PHASE) + return -EOPNOTSUPP; + tmr_idx = hw->func_caps.ts_func_info.tmr_index_owned; /* 0. Reset mode & out_en in AUX_OUT */ @@ -1836,7 +1850,6 @@ ice_ptp_gpio_enable_e810(struct ptp_clock_info *info, bool sma_pres = false; unsigned int chan; u32 gpio_pin; - int err; if (ice_is_feature_supported(pf, ICE_F_SMA_CTRL)) sma_pres = true; @@ -1865,14 +1878,14 @@ ice_ptp_gpio_enable_e810(struct ptp_clock_info *info, clk_cfg.gpio_pin = chan; } + clk_cfg.flags = rq->perout.flags; clk_cfg.period = ((rq->perout.period.sec * NSEC_PER_SEC) + rq->perout.period.nsec); clk_cfg.start_time = ((rq->perout.start.sec * NSEC_PER_SEC) + rq->perout.start.nsec); clk_cfg.ena = !!on; - err = ice_ptp_cfg_clkout(pf, chan, &clk_cfg, true); - break; + return ice_ptp_cfg_clkout(pf, chan, &clk_cfg, true); } case PTP_CLK_REQ_EXTTS: { @@ -1897,14 +1910,12 @@ ice_ptp_gpio_enable_e810(struct ptp_clock_info *info, extts_cfg.gpio_pin = gpio_pin; extts_cfg.ena = !!on; - ice_ptp_cfg_extts(pf, chan, &extts_cfg, true); - return 0; + + return ice_ptp_cfg_extts(pf, chan, &extts_cfg, true); } default: return -EOPNOTSUPP; } - - return err; } /** @@ -1917,19 +1928,18 @@ static int ice_ptp_gpio_enable_e823(struct ptp_clock_info *info, struct ptp_clock_request *rq, int on) { struct ice_pf *pf = ptp_info_to_pf(info); - int err; switch (rq->type) { case PTP_CLK_REQ_PPS: { struct ice_perout_channel clk_cfg = {}; + clk_cfg.flags = rq->perout.flags; clk_cfg.gpio_pin = PPS_PIN_INDEX; clk_cfg.period = NSEC_PER_SEC; clk_cfg.ena = !!on; - err = ice_ptp_cfg_clkout(pf, PPS_CLK_GEN_CHAN, &clk_cfg, true); - break; + return ice_ptp_cfg_clkout(pf, PPS_CLK_GEN_CHAN, &clk_cfg, true); } case PTP_CLK_REQ_EXTTS: { @@ -1939,14 +1949,11 @@ static int ice_ptp_gpio_enable_e823(struct ptp_clock_info *info, extts_cfg.gpio_pin = TIME_SYNC_PIN_INDEX; extts_cfg.ena = !!on; - ice_ptp_cfg_extts(pf, rq->extts.index, &extts_cfg, true); - break; + return ice_ptp_cfg_extts(pf, rq->extts.index, &extts_cfg, true); } default: return -EOPNOTSUPP; } - - return err; } /** diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h index f1171cdd93c8..e2af9749061c 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h @@ -29,6 +29,7 @@ enum ice_ptp_pin_e810t { struct ice_perout_channel { bool ena; u32 gpio_pin; + u32 flags; u64 period; u64 start_time; };