netdev - Re: [PATCH v7 iwl-next 1/6] ice: Remove unncecessary ice_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <IA0PR11MB838047BDBD2DE7C379A6A3C686942@IA0PR11MB8380.namprd11.prod.outlook.com>
Date: Tue, 27 Aug 2024 11:32:58 +0000
From: "Kolacinski, Karol" <karol.kolacinski@...el.com>
To: Simon Horman <horms@...nel.org>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH v7 iwl-next 1/6] ice: Remove unncecessary ice_is_e8xx()
 functions

On Fri, Aug 23, 2024 at 10:15PM +0200, Simon Horman wrote:
>> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
>> index f02e8ca55375..dd65b2db9856 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
>> @@ -386,27 +386,22 @@ void ice_gnss_exit(struct ice_pf *pf)
>>   */
>>  bool ice_gnss_is_gps_present(struct ice_hw *hw)
>>  {
>> +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
>> +     int err;
>> +     u8 data;
>> +
>>        if (!hw->func_caps.ts_func_info.src_tmr_owned)
>>                return false;
>> 
>>        if (!ice_is_gps_in_netlist(hw))
>>                return false;
>> 
>> -#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
>> -     if (ice_is_e810t(hw)) {
>> -             int err;
>> -             u8 data;
>> -
>> -             err = ice_read_pca9575_reg(hw, ICE_PCA9575_P0_IN, &data);
>> -             if (err || !!(data & ICE_P0_GNSS_PRSNT_N))
>> -                     return false;
>> -     } else {
>> +     err = ice_read_pca9575_reg(hw, ICE_PCA9575_P0_IN, &data);
>> +     if (err || !!(data & ICE_P0_GNSS_PRSNT_N))
>>                return false;
>> -     }
>> -#else
>> -     if (!ice_is_e810t(hw))
>> -             return false;
>> -#endif /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */
>> 
>>        return true;
>> +#else
>> +     return false;
>> +#endif /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */
>>  }
> 
> I understand that the above relates to the patch description in the sense
> that it removes calls to ice_is_e810t(), a function that is removed
> entirely by this patch. But the above is not a simple substitution of one
> check for E810T for another. Indeed, it seems far more complex than that.
> 
> I think that warrants an explanation, e.g. why is it ok to always return
> false if CONFIG_PTP_1588_CLOCK. Perhaps a separate patch is appropriate for
> this change.

Sure, I'll separate GNSS change from the rest of the patch.

>> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> ...
> 
>> @@ -2759,7 +2766,7 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
>>        bool trigger_oicr = false;
>>        unsigned int i;
>> 
>> -     if (ice_is_e810(hw))
>> +     if (!pf->ptp.port.tx.has_ready_bitmap)
>>                return;
>> 
>>        if (!ice_pf_src_tmr_owned(pf))
> 
> Likewise, this doesn't really match the patch description.
> Sure it is a simple change. And yes, after scanning the code,
> I agree that has_ready_bitmap is set other than for E810.
> But it is not a check against the MAC type as described in the patch
> description.
> 
> A separate patch would be nice. Or, if not, an enhanced patch description.

I'll add patch description for this one-liner.

>> @@ -2898,14 +2905,12 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
>>         */
>>        ice_ptp_flush_all_tx_tracker(pf);
>> 
>> -     if (!ice_is_e810(hw)) {
>> -             /* Enable quad interrupts */
>> -             err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
>> -             if (err)
>> -                     return err;
>> +     /* Enable quad interrupts */
>> +     err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
>> +     if (err)
>> +             return err;
>> 
>> -             ice_ptp_restart_all_phy(pf);
>> -     }
>> +     ice_ptp_restart_all_phy(pf);
>> 
>>        /* Re-enable all periodic outputs and external timestamp events */
>>        ice_ptp_enable_all_perout(pf);
> 
> Here too. Why is it ok to unconditionally enable quad interrupts?

All functions called in ice_ptp_cfg_phy_interrupt() check the MAC type
anyway, so there's no need to check it here.

Thanks,
Karol