| Message ID | 1319178708-10881-2-git-send-email-clchiou@chromium.org |
|---|---|
| State | Deferred |
| Delegated to: | Detlev Zundel |
| Headers | show |
Dear Che-Liang Chiou, In message <1319178708-10881-2-git-send-email-clchiou@chromium.org> you wrote: > The block_dev_desc struct has #ifdef on lba48 and variable-size on lba > and so its layout varies from config to config. At least part_efi.c has > partially complained about this. > > This patch makes lba48 be always defined and lba be fixed to largest > size that an LBA would need so that the block_dev_desc layout would be > an invariant with respect to configurations. > > Doing so would waste a few extra bytes per struct block_dev_desc, which > I believe is not critical. How much exactly is "a few bytes"? Best regards, Wolfgang Denk
Dear Wolfgang Denk, I guess I have to put this patchset on hold. I will get you back if we could proceed with this patchset. Regards, Che-Liang On Sat, Oct 22, 2011 at 3:09 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Che-Liang Chiou, > > In message <1319178708-10881-2-git-send-email-clchiou@chromium.org> you wrote: >> The block_dev_desc struct has #ifdef on lba48 and variable-size on lba >> and so its layout varies from config to config. At least part_efi.c has >> partially complained about this. >> >> This patch makes lba48 be always defined and lba be fixed to largest >> size that an LBA would need so that the block_dev_desc layout would be >> an invariant with respect to configurations. >> >> Doing so would waste a few extra bytes per struct block_dev_desc, which >> I believe is not critical. > > How much exactly is "a few bytes"? > > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > As long as we're going to reinvent the wheel again, we might as well > try making it round this time. - Mike Dennison >
Hi Che-liang, > I guess I have to put this patchset on hold. I will get you back if we > could proceed with this patchset. Please don't top-post. The mails really are more difficult to read in context. > Regards, > Che-Liang > > On Sat, Oct 22, 2011 at 3:09 AM, Wolfgang Denk <wd@denx.de> wrote: >> Dear Che-Liang Chiou, >> >> In message <1319178708-10881-2-git-send-email-clchiou@chromium.org> you wrote: >>> The block_dev_desc struct has #ifdef on lba48 and variable-size on lba >>> and so its layout varies from config to config. At least part_efi.c has >>> partially complained about this. >>> >>> This patch makes lba48 be always defined and lba be fixed to largest >>> size that an LBA would need so that the block_dev_desc layout would be >>> an invariant with respect to configurations. >>> >>> Doing so would waste a few extra bytes per struct block_dev_desc, which >>> I believe is not critical. >> >> How much exactly is "a few bytes"? As far as I can see, the difference is 4 bytes _and_ it is a runtime data structure, so it should not make any difference for the code size. Che-liang can surely correct me if I'm wrong. Moreover it seems we need to do something comparable sooner or later. If we want to support large block devices and the partition code uses block devices, that code needs to be prepared to work with that. So in general I'm in favor of doing something like this. On the other hand, the patch changes the datatype of a field which gets used in lots of places - Che-liang, did you run a MAKEALL with this patch and check that no more warnings/errors are introduced? Cheers Detlev
diff --git a/disk/part_dos.c b/disk/part_dos.c index b5bcb37..a0938db 100644 --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -119,7 +119,7 @@ static void print_partition_extended (block_dev_desc_t *dev_desc, int ext_part_s return; } if(i==DOS_PBR) { - printf (" 1\t\t 0\t%10ld\t%2x\n", + printf (" 1\t\t 0\t%10lld\t%2x\n", dev_desc->lba, buffer[DOS_PBR_MEDIA_TYPE_OFFSET]); return; } diff --git a/disk/part_efi.c b/disk/part_efi.c index 0a513c6..e779dc1 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -22,10 +22,8 @@ */ /* - * Problems with CONFIG_SYS_64BIT_LBA: - * * struct disk_partition.start in include/part.h is sized as ulong. - * When CONFIG_SYS_64BIT_LBA is activated, lbaint_t changes from ulong to uint64_t. + * struct block_dev_desc.lba in the same header is sized as uint64_t. * For now, it is cast back to ulong at assignment. * * This limits the maximum size of addressable storage to < 2 Terra Bytes diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 391bc2b..c17e495 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -265,7 +265,7 @@ mmc_write_blocks(struct mmc *mmc, ulong start, lbaint_t blkcnt, const void*src) int timeout = 1000; if ((start + blkcnt) > mmc->block_dev.lba) { - printf("MMC: block number 0x%lx exceeds max(0x%lx)\n", + printf("MMC: block number 0x%lx exceeds max(0x%llx)\n", start + blkcnt, mmc->block_dev.lba); return 0; } @@ -393,7 +393,7 @@ static ulong mmc_bread(int dev_num, ulong start, lbaint_t blkcnt, void *dst) return 0; if ((start + blkcnt) > mmc->block_dev.lba) { - printf("MMC: block number 0x%lx exceeds max(0x%lx)\n", + printf("MMC: block number 0x%lx exceeds max(0x%llx)\n", start + blkcnt, mmc->block_dev.lba); return 0; } diff --git a/drivers/mmc/pxa_mmc.c b/drivers/mmc/pxa_mmc.c index 48e21ef..67c33d4 100644 --- a/drivers/mmc/pxa_mmc.c +++ b/drivers/mmc/pxa_mmc.c @@ -541,7 +541,7 @@ static void mmc_decode_csd(uint32_t * resp) mmc_dev.removable = 0; mmc_dev.block_read = mmc_bread; - printf("Detected: %lu blocks of %lu bytes (%luMB) ", + printf("Detected: %llu blocks of %lu bytes (%lluMB) ", mmc_dev.lba, mmc_dev.blksz, mmc_dev.lba * mmc_dev.blksz / (1024 * 1024)); diff --git a/include/part.h b/include/part.h index 1827767..be0a22e 100644 --- a/include/part.h +++ b/include/part.h @@ -33,10 +33,8 @@ typedef struct block_dev_desc { unsigned char lun; /* target LUN */ unsigned char type; /* device type */ unsigned char removable; /* removable device */ -#ifdef CONFIG_LBA48 unsigned char lba48; /* device can use 48bit addr (ATA/ATAPI v7) */ -#endif - lbaint_t lba; /* number of blocks */ + uint64_t lba; /* number of blocks */ unsigned long blksz; /* block size */ char vendor [40+1]; /* IDE model, SCSI Vendor */ char product[20+1]; /* IDE Serial no, SCSI product */
The block_dev_desc struct has #ifdef on lba48 and variable-size on lba and so its layout varies from config to config. At least part_efi.c has partially complained about this. This patch makes lba48 be always defined and lba be fixed to largest size that an LBA would need so that the block_dev_desc layout would be an invariant with respect to configurations. Doing so would waste a few extra bytes per struct block_dev_desc, which I believe is not critical. Signed-off-by: Che-Liang Chiou <clchiou@chromium.org> --- disk/part_dos.c | 2 +- disk/part_efi.c | 4 +--- drivers/mmc/mmc.c | 4 ++-- drivers/mmc/pxa_mmc.c | 2 +- include/part.h | 4 +--- 5 files changed, 6 insertions(+), 10 deletions(-)