linux-kernel - Re: HPA patches

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20070323130315.7ed48121.randy.dunlap@oracle.com>
Date:	Fri, 23 Mar 2007 13:03:15 -0700
From:	Randy Dunlap <randy.dunlap@...cle.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org,
	kyle@...onical.com
Subject: Re: HPA patches

On Fri, 23 Mar 2007 20:08:19 +0000 Alan Cox wrote:

> > > +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.

dependent.  OK, thanks.

> > > +	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. 

Already corrected (printk types).  And putting
					hpa_sectors);
on a separate line doesn't hurt readability.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/