netdev - Re: [03/22] Cyclades PC300 driver: Fix usage of types

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: <m3hazb3dii.fsf@intrepid.localdomain>
Date:	Tue, 31 Jan 2012 19:58:29 +0100
From:	Krzysztof Halasa <khc@...waw.pl>
To:	Andrea Shepard <andrea@...sephoneslair.org>
Cc:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	davem@...emloft.net, mmarek@...e.cz, jkosina@...e.cz,
	joe@...ches.com, justinmattock@...il.com, gregkh@...e.de,
	alan@...ux.intel.com, jdmason@...zu.us
Subject: Re: [03/22] Cyclades PC300 driver: Fix usage of types

Hi,

Andrea Shepard <andrea@...sephoneslair.org> writes:

> +++ b/drivers/net/wan/hd64572.h
> @@ -172,11 +172,11 @@
>  
>  /* Block Descriptor Structure */
>  typedef struct {
> -	unsigned long	next;		/* pointer to next block descriptor */
> -	unsigned long	ptbuf;		/* buffer pointer */
> -	unsigned short	len;		/* data length */
> -	unsigned char	status;		/* status */
> -	unsigned char	filler[5];	/* alignment filler (16 bytes) */ 
> +	u32	next;		/* pointer to next block descriptor */
> +	u32	ptbuf;		/* buffer pointer */
> +	u16	len;		/* data length */
> +	u8	status;		/* status */
> +	u8	filler[5];	/* alignment filler (16 bytes) */
>  } pcsca_bd_t;

Perhaps you could just use the following struct which is already in the
file:

/* Block Descriptor Structure */
typedef struct {
        u32 cp;                 /* pointer to next block descriptor */
        u32 bp;                 /* buffer pointer */
        u16 len;                /* data length */
        u8 stat;                /* status */
        u8 unused;              /* pads to 4-byte boundary */
}pkt_desc;

The difference is a different alignment (4 bytes vs 16 bytes) but
I think it's been verified (both docs and real hw) that 32-bit alignment
is ok with these chips.

> +#define cpc_writeb(port, val)	{writeb((u8)(val), \
> +				(void __iomem *)(port)); mb(); }
> +#define cpc_writew(port, val)	{writew((u16)(val), \
> +				(void __iomem *)(port)); mb(); }
> +#define cpc_writel(port, val)	{writel((u32)(val), \
> +				(void __iomem *)(port)); mb(); }

Are these mb() needed? I think write[bwl] and read[bwl] are already
memory barriers.
PCI write posting is a different thing of course and you may need e.g.
a PCI read in some timing-critical parts of the code.

>  #else /* __KERNEL__ */
> -#define cpc_writeb(port,val)	(*(volatile ucchar *)(port) = (ucchar)(val))
> -#define cpc_writew(port,val)	(*(volatile ucshort *)(port) = (ucshort)(val))
> -#define cpc_writel(port,val)	(*(volatile uclong *)(port) = (uclong)(val))
> +#define cpc_writeb(port, val)	(*(u8 *)(port) = (u8)(val))
> +#define cpc_writew(port, val)	(*(u16 *)(port) = (u16)(val))
> +#define cpc_writel(port, val)	(*(u32 *)(port) = (u32)(val))
>  
> -#define cpc_readb(port)		(*(volatile ucchar *)(port))
> -#define cpc_readw(port)		(*(volatile ucshort *)(port))
> -#define cpc_readl(port)		(*(volatile uclong *)(port))
> +#define cpc_readb(port)		(*(u8 *)(port))
> +#define cpc_readw(port)		(*(u16 *)(port))
> +#define cpc_readl(port)		(*(u32 *)(port))

I'm not sure the kernel headers are a good place for such things.

>  void tx_dma_stop(pc300_t * card, int ch)
>  {
> -	uclong scabase = card->hw.scabase;
> -	ucchar drr_ena_bit = 1 << (5 + 2 * ch);
> -	ucchar drr_rst_bit = 1 << (1 + 2 * ch);
> +	uintptr_t scabase = (uintptr_t)(card->hw.scabase);
> +	u8 drr_ena_bit = 1 << (5 + 2 * ch);
> +	u8 drr_rst_bit = 1 << (1 + 2 * ch);

uintptr_t? Is it simply an unsigned int *?
-- 
Krzysztof Halasa
--
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