Re: [PATCH] Sgpio support in sata_nv

[Andrew Morton <akpm@osdl.org>]

On Mon, 21 Aug 2006 18:17:06 -0700
Prajakta Gudadhe <pgudadhe@nvidia.com> wrote:

> Description:
> Added support for enclosure management via SGPIO to sata_nv. This patch is based off of kernel-2.6.17.9.
> 
> Signed-off by: Prajakta Gudadhe <pgudadhe@nvidia.com>
> 
> 
> +union nv_sgpio_csr
> +{
> +	struct {
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> +		u8	sgpio_status:2;
> +		u8	sgpio_seq:1;
> +		u8	cmd_status:2;
> +		u8	cmd:3;
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> +		u8	cmd:3;
> +		u8	cmd_status:2;
> +		u8	sgpio_seq:1;
> +		u8	sgpio_status:2;
> +#else
> +#error "Please fix <asm/byteorder.h>"
> +#endif
> +	} bit;
> +	u8	all;
> +};

I believe it's still unfashoinable to attempt to map hardware registers
onto compiler-controlled bitfields in this manner.

I'd suggest that you just pull the u32 out of PCI space and open-code the
shifting and masking in an endianness-independent fashion.  The macros
around line 508 of drivers/net/3c59x.c demonstrate one way of doing this.

> +static int nv_port_start(struct ata_port *ap)
> +{
> +	int stat;
> +    	struct nv_port *port;
> +
> +    	stat = ata_port_start(ap);
> +    	if (stat) {
> +        	return stat;
> +    	}
> +
> +    	port = kmalloc(sizeof(struct nv_port), GFP_KERNEL);
> +    	if (!port) 
> +		goto err_out_no_free;
> +
> +	memset(port, 0, sizeof(struct nv_port));

kzalloc().

> +    	ap->private_data = port;
> +    	return 0;
> +
> +err_out_no_free:
> +    	return 1;
> +}
> +
> +
>
> ..
>
>  static void nv_enable_hotplug(struct ata_probe_ent *probe_ent)
>  {
>  	u8 intr_mask;
> @@ -606,6 +877,238 @@ static int nv_check_hotplug_ck804(struct
>  	return 0;
>  }
>  
> +static void nv_sgpio_init(struct pci_dev *pdev, struct nv_host *phost)
> +{
> +    	u16 csr_add; 
> +	u32 cb_add, temp32;
> +	struct device *dev = pci_dev_to_dev(pdev);
> +	struct ata_host_set *host_set = dev_get_drvdata(dev);
> +
> +	pci_read_config_word(pdev, NV_SGPIO_PCI_CSR_OFFSET, &csr_add);
> +	pci_read_config_dword(pdev, NV_SGPIO_PCI_CB_OFFSET, &cb_add);
> +    	if (csr_add == 0 || cb_add == 0) {
> +        	return;
> +    	}
> +
> +	temp32 = csr_add;

temp32 came from a pci config space read.

> +    	phost->host_sgpio.pcsr = (union nv_sgpio_csr *)temp32;

And we copy that into a kernel pointer??  Really?

> +    	phost->host_sgpio.pcb = phys_to_virt(cb_add);
> +
> +    	if (phost->host_sgpio.pcb->scratch_space == 0) {
> +        	spin_lock_init(&nv_sgpio_lock);
> +        	phost->host_sgpio.share.plock = &nv_sgpio_lock;
> +        	phost->host_sgpio.share.ptstamp = &nv_sgpio_tstamp;
> +		phost->host_sgpio.pcb->scratch_space = 
> +			(unsigned long)&phost->host_sgpio.share;
> +        	spin_lock(phost->host_sgpio.share.plock);
> +        	nv_sgpio_reset(phost->host_sgpio.pcsr);
> +        	phost->host_sgpio.pcb->cr0.bit.enable = 1;
> +		spin_unlock(phost->host_sgpio.share.plock);
> +    	}
> +
> +    	phost->host_sgpio.share = 
> +		*(struct nv_sgpio_host_share *)(unsigned long)
> +		phost->host_sgpio.pcb->scratch_space;
> +    	phost->host_sgpio.flags.sgpio_enabled = 1;
> +
> +    	init_timer(&phost->host_sgpio.sgpio_timer);
> +    	phost->host_sgpio.sgpio_timer.data = (unsigned long)host_set;
> +    	nv_sgpio_set_timer(&phost->host_sgpio.sgpio_timer, 
> +				NV_SGPIO_UPDATE_TICK);
> +}
> +
>
> ...
>
> +
> +static void nv_sgpio_timer_handler(unsigned long context)
> +{
> +
> +    	struct ata_host_set *host_set = (struct ata_host_set *)context;
> +    	struct nv_host *host;
> +    	u8 count, host_offset, port_offset;
> +    	union nv_sgpio_tx tx;
> +    	bool on_off;
> +    	unsigned long mask = 0xFFFF;
> +	struct nv_port *port;
> +
> +    	if (!host_set)
> +		goto err_out;
> +	else 
> +		host = (struct nv_host *)host_set->private_data;

ata_host_set.parivate_data is void*, so this cast is unneeded.

> +	if (!host->host_sgpio.flags.sgpio_enabled)
> +	        goto err_out;
> +
> +	host_offset = nv_sgpio_tx_host_offset(host_set);
> +
> +    	spin_lock(host->host_sgpio.share.plock);
> +    	tx = host->host_sgpio.pcb->tx[host_offset];
> +    	spin_unlock(host->host_sgpio.share.plock);
> +
> +    	for (count = 0; count < host_set->n_ports; count++) {
> +        	struct ata_port *ap; 
> +
> +        	ap = host_set->ports[count];
> +        
> +        	if (!(ap && !(ap->flags & ATA_FLAG_PORT_DISABLED)))
> +			continue;
> +
> +            	port = (struct nv_port *)ap->private_data;

Ditto.

> +		if (!port)
> +			continue;            		
> +                port_offset = nv_sgpio_tx_port_offset(ap);

whitepsace went funny.

> +	        on_off = tx.bit.tx_port[port_offset].bit.activity;
> +         	if (nv_sgpio_update_led(&port->port_sgpio.activity, &on_off)) {
> +                    	tx.bit.tx_port[port_offset].bit.activity = on_off;
> +                    	host->host_sgpio.flags.need_update = 1;
> +                }

Ditto.  In fact in many places this patch uses spaces where it should be
using tabs.

> ...
>

	if (jiffies_to_msecs(jiffies - *ptstamp) >= NV_SGPIO_MIN_UPDATE_DELTA) {

I think this works OK in the presence of jiffies wraparound.  But it would
be more idiomatic to do

	if (time_after(jiffies,
		*ptstamp + msecs_to_jiffies(NV_SGPIO_MIN_UPDATE_DELTA)) {


> ...
>
> +
> +static bool nv_sgpio_update_led(struct nv_sgpio_led *led, bool *on_off)

Please remove the new private implementation of `bool' and just use `int'. 
There's ongoing discussion about how to do a kernel-wide implementation of
bool, and adding new driver-private ones now just complicates that.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/