From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [PATCH] 2.6.35 libata support for > 512 byte sectors (e.g. 4K Native) Date: Mon, 16 Aug 2010 15:41:09 -0400 Message-ID: <4C699455.6020200@pobox.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Grant Grundler Cc: "Wilcox, Matthew R" , Tejun Heo , Linux IDE , Gwendal Grignou , LKML List-Id: linux-ide@vger.kernel.org On 08/16/2010 03:17 PM, Grant Grundler wrote: > On Sat, Aug 14, 2010 at 9:37 PM, Wilcox, Matthew R > wrote: >> If you will insist on sending to my Outlook address, you'll get replies in standard broken Outlook format. You've been warned. > > You trumped my Gmail warning. I fold. :) > >> >> --- >> >> +/* BUG: Big endian systems need accesses to "id" wrapped with le16_to_cpu(). >> + */ >> >> No, it's already been converted. See ata_dev_read_id(). > > Ah - good. I'll remove the comment. > >> >> I'm not sure about your use of a switch to set the sector size. Have you checked the code that GCC generates for this? > > The switch probably sucks unless we could weight the order of the > tests. E.g. common cases first. But it's just an implementation detail > that is relatively easy to replace with the bitmap you had implemented > before. > >> >> All the places you dereference dev->sdev are within a callchain from ->queuecommand; sdev can't possibly go away. >> It'd be clearer that this is the case if you used scmd->device->sector_size instead of dev->sdev->sector_size. > > Thank you - that looks much better to me too. > >> -- >> >> @@ -516,7 +515,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg) >> memset(scsi_cmd, 0, sizeof(scsi_cmd)); >> >> if (args[3]) { >> - argsize = SECTOR_SIZE * args[3]; >> + argsize = ATA_SECT_SIZE * args[3]; >> argbuf = kmalloc(argsize, GFP_KERNEL); >> if (argbuf == NULL) { >> rc = -ENOMEM; >> >> I think this is wrong. The ioctl does PIO Data-in; as such, it should use the native sector size, not 512. >> That said, there's a possibility of data corruption for programs which use this ioctl, assuming a 512-byte sector size when it's natively 4k. >> This one's tricky and needs serious thought. I might error it if sector_size isn't 512 bytes :-) >> It's a legacy ioctl anyway, right? > > I have no idea. If it's tricky, I probably have it wrong. > Anyone else have guidance here? The main question is whether the size of a DRQ block changes, when LBA logical size changes? I need to review the ATA8 specs in this area, but I would think some interfaces that return 512-byte pages for things like SMART info would be unchanged. How do the drives behave for PIO-Data-{In,Out} commands that are not reading/writing user data, but rather drive metadata? Jeff