netdev - Re: [PATCH net-next v2] amd-xgbe: Add PPS periodic output support

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: <9a9e7815-4af9-4e47-8137-ffe9602da06e@amd.com>
Date: Thu, 28 Aug 2025 13:14:45 +0530
From: "Rangoju, Raju" <raju.rangoju@....com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, pabeni@...hat.com, richardcochran@...il.com,
 linux-kernel@...r.kernel.org, Shyam-sundar.S-k@....com
Subject: Re: [PATCH net-next v2] amd-xgbe: Add PPS periodic output support



On 8/26/2025 5:34 AM, Jakub Kicinski wrote:
> On Fri, 22 Aug 2025 16:08:31 +0530 Raju Rangoju wrote:
>> +#define PPSx_MASK(x) ({						\
>> +	unsigned int __x = (x);					\
>> +	GENMASK(PPS_MAXIDX(__x), PPS_MINIDX(__x));		\
>> +})
>> +#define PPSCMDx(x, val) ({					\
>> +	unsigned int __x = (x);					\
>> +	GENMASK(PPS_MINIDX(__x) + 3, PPS_MINIDX(__x)) &		\
>> +	((val) << PPS_MINIDX(__x));				\
>> +})
>> +#define TRGTMODSELx(x, val) ({					\
>> +	unsigned int __x = (x);					\
>> +	GENMASK(PPS_MAXIDX(__x) - 1, PPS_MAXIDX(__x) - 2) &	\
>> +	((val) << (PPS_MAXIDX(__x) - 2));			\
>> +})
> 
> These macros are way too gnarly, please simplify them.
> For a start I'm not sure why you're making your life harder and try
> to have a shifted mask. Instead of masking the value before shifting.

Sure, will simplify them v3.

> 
>>   static int xgbe_enable(struct ptp_clock_info *info,
>>   		       struct ptp_clock_request *request, int on)
>>   {
>> -	return -EOPNOTSUPP;
>> +	struct xgbe_prv_data *pdata = container_of(info, struct xgbe_prv_data,
>> +						   ptp_clock_info);
>> +	struct xgbe_pps_config *pps_cfg;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	dev_dbg(pdata->dev, "rq->type %d on %d\n", request->type, on);
>> +
>> +	if (request->type != PTP_CLK_REQ_PEROUT)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* Reject requests with unsupported flags */
>> +	if (request->perout.flags)
>> +		return -EOPNOTSUPP;
> 
> Are you sure kernel can actually send you any flags here?
> ops->supported_perout_flags exists

Yes, it looks like ops->supported_perout_flags is already taking care of 
this. Thanks, I'll drop the above check.

> 
>> +	/* Validate index against our limit */
>> +	if (request->perout.index >= XGBE_MAX_PPS_OUT)
>> +		return -EINVAL;
> 
> Are you sure kernel can send you an index higher than what driver
> registered as supported?

looks like kernel is already handling it and this extra check isn't 
needed anymore. I'll drop it. Thanks Jakub.