netdev - Re: [PATCH net-next v2 1/1] net: fec: Add VLAN receive HW 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: <51D29FD3.5030804@mentor.com>
Date:	Tue, 2 Jul 2013 10:39:31 +0100
From:	Jim Baxter <jim_baxter@...tor.com>
To:	David Miller <davem@...emloft.net>
CC:	<Frank.Li@...escale.com>, <B38611@...escale.com>,
	<fabio.estevam@...escale.com>, <l.stach@...gutronix.de>,
	<shawn.guo@...aro.org>, <netdev@...r.kernel.org>,
	<bhutchings@...arflare.com>
Subject: Re: [PATCH net-next v2 1/1] net: fec: Add VLAN receive HW support.

On 02/07/13 01:09, David Miller wrote:
> From: Jim Baxter <jim_baxter@...tor.com>
> Date: Fri, 28 Jun 2013 15:08:23 +0100
> 
>> @@ -803,6 +807,9 @@ fec_enet_rx(struct net_device *ndev, int budget)
>>  	ushort	pkt_len;
>>  	__u8 *data;
>>  	int	pkt_received = 0;
>> +	struct	bufdesc_ex *ebdp = NULL;
>> +	bool	vlan_packet_rcvd = false;
>> +	u16	vlan_tag;
>>  
>>  #ifdef CONFIG_M532x
>>  	flush_cache_all();
>> @@ -866,6 +873,24 @@ fec_enet_rx(struct net_device *ndev, int budget)
>>  		if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME)
>>  			swap_buffer(data, pkt_len);
>>  
>> +		/* Extract the enhanced buffer descriptor */
>> +		ebdp = NULL;
>> +		if (fep->bufdesc_ex)
>> +			ebdp = (struct bufdesc_ex *)bdp;
> 
> I would use a union here, so you'd have something like:
> 
> 	union {
> 		struct bufdesc		*bdp;
> 		struct bufdesc_ex	*bdp_ex;
> 	} *p;
> 
> Alternatively, you can always use "struct bufdesc_ex *p", along with
> the boolean saying if the extended descriptors are in use.
> 

I see (I think) I would replace the variables:
struct bufdesc *bdp;
struct	bufdesc_ex *ebdp

with a single union variable *p and then access p.bdp where the *bdp  is
currently used and use the fep->bufdesc_ex flag to determine if p.bdp_ex
can be accessed instead of my *ebdp variable.


>> +		if ((ndev->features & NETIF_F_HW_VLAN_CTAG_RX) &&
>> +				ebdp && (ebdp->cbd_esc & BD_ENET_RX_VLAN)) {
> 
> This is not indented properly.   The "ebp" on that second line must line
> up exactly with the first column after the openning parenthesis on the
> first line.
> 
> I see what you're trying to do, purely using TAB characters to indent
> that second line.  But you must take the care and time to use the
> appropriate number of TAB and space characters to place things at the
> proper column.
> 

Thank you, I will do this correctly in future.

>> +			skb_copy_to_linear_data_offset(skb, (2 * ETH_ALEN),
>> +					data + payload_offset,
>> +					pkt_len - 4 - (2 * ETH_ALEN));
> 
> Again, line up the first non-space character on the second and third
> lines of this function call so that they line up to the first column
> after the openning parenthesis of the first line.
> 
>> +				__vlan_hwaccel_put_tag(skb,
>> +						htons(ETH_P_8021Q), vlan_tag);
> 
> Likewise.
> 

Jim
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html