Re: HPA patches

[Alan Cox <alan@lxorguk.ukuu.org.uk>]

> > +static int ata_ignore_hpa = 0;
> 
> Don't init to 0.  Not needed, bloats binary files.

It'll be one for the final release 8)

> > +module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
> > +MODULE_PARM_DESC(ignore_hpa, "Ignore HPA (0=off 1=on)");
> 
> So 1 = on = ignore, right?

Yes.

> > +	tf.command = ATA_CMD_READ_NATIVE_MAX_EXT;
> > +	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 | ATA_TFLAG_ISADDR;
> > +	tf.protocol |= ATA_PROT_NODATA;
> > +	tf.device = 0x40;
> 
> What is 0x40?  can it be #defined (or enum-ed) instead of a magic
> value?  please?  (more of same below)

It's 0x40. Its a "command dependant bit" - no useful name.

> > +	u64 sectors = dev->n_sectors;
> > +	u64 hpa_sectors;
> > +	
> > +	if (ata_id_has_lba48(dev->id))
> > +		hpa_sectors = ata_read_native_max_address_ext(dev);
> > +	else
> > +		hpa_sectors = ata_read_native_max_address(dev);
> > +
> > +	/* if no hpa, both should be equal */
> > +	ata_dev_printk(dev, KERN_INFO, "%s 1: sectors = %lld, hpa_sectors = %lld\n",
> > +		__FUNCTION__, sectors, hpa_sectors);
> 
> (long long) or (unsigned long long) on sectors and hpa_sectors...

u64 is always unsigned long long (and its debug anyway)

> 
> > +
> > +	if (hpa_sectors > sectors) {
> > +		ata_dev_printk(dev, KERN_INFO,
> > +			"Host Protected Area detected:\n"
> > +			"\tcurrent size: %lld sectors\n"
> > +			"\tnative size: %lld sectors\n",
> > +			sectors, hpa_sectors);
> 
> printk format types ok?

Yes

> > +			if (hpa_sectors) {
> > +				ata_dev_printk(dev, KERN_INFO,
> > +					"native size increased to %lld sectors\n", hpa_sectors);
> 
> Line lengths < 80 and printk format types?

See above, and the 80 column fascists can suffer in the name of
readability. 

Alan