[U-Boot] [PATCH 0/2] api: extend accessible set of block device attributes
struct device_info in api_public.h defined its own subset of attributes of block_dev_desc, which limits the capability of external apps.
This patch set let external apps access the same set of block device attributes as U-Boot.
Che-Liang Chiou (2): Flatten and solidify block_dev_desc layout api: storage: Share attributes with block_dev_desc
api/api_storage.c | 17 +++++++++++++++-- disk/part_dos.c | 2 +- disk/part_efi.c | 4 +--- drivers/mmc/mmc.c | 4 ++-- drivers/mmc/pxa_mmc.c | 2 +- examples/api/demo.c | 15 +++++++++++++-- include/api_public.h | 8 +------- include/block_dev_attr.h | 39 +++++++++++++++++++++++++++++++++++++++ include/part.h | 18 +++--------------- 9 files changed, 76 insertions(+), 33 deletions(-) create mode 100644 include/block_dev_attr.h
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(-)
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 */
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
struct device_info in api_public.h defined its own subset of attributes of block_dev_desc, which limits the capability of external apps.
This patch let struct device_info and block_dev_desc share the same set of attributes so that an external app has equal amount of information of block device compared to U-Boot.
The export.h and _export.h have somewhat addressed the same issue. That is, sharing declarations between U-Boot and external apps.
Signed-off-by: Che-Liang Chiou clchiou@chromium.org --- api/api_storage.c | 17 +++++++++++++++-- examples/api/demo.c | 15 +++++++++++++-- include/api_public.h | 8 +------- include/block_dev_attr.h | 39 +++++++++++++++++++++++++++++++++++++++ include/part.h | 16 +++------------- 5 files changed, 71 insertions(+), 24 deletions(-) create mode 100644 include/block_dev_attr.h
diff --git a/api/api_storage.c b/api/api_storage.c index c535712..ab4cad5 100644 --- a/api/api_storage.c +++ b/api/api_storage.c @@ -165,8 +165,21 @@ static int dev_stor_get(int type, int first, int *more, struct device_info *di) debugf("device instance exists, but is not active.."); found = 0; } else { - di->di_stor.block_count = dd->lba; - di->di_stor.block_size = dd->blksz; +#define COPY_ATTR(a) di->di_stor.a = dd->a + COPY_ATTR(if_type); + COPY_ATTR(dev); + COPY_ATTR(part_type); + COPY_ATTR(target); + COPY_ATTR(lun); + COPY_ATTR(type); + COPY_ATTR(removable); + COPY_ATTR(lba48); + COPY_ATTR(lba); + COPY_ATTR(blksz); +#undef COPY_ATTR + strcpy(di->di_stor.vendor, dd->vendor); + strcpy(di->di_stor.product, dd->product); + strcpy(di->di_stor.revision, dd->revision); } }
diff --git a/examples/api/demo.c b/examples/api/demo.c index 65e7491..0c65ae9 100644 --- a/examples/api/demo.c +++ b/examples/api/demo.c @@ -294,7 +294,18 @@ void test_dump_di(int handle)
} else if (di->type & DEV_TYP_STOR) { printf(" type\t\t= %s\n", test_stor_typ(di->type)); - printf(" blk size\t\t= %d\n", (unsigned int)di->di_stor.block_size); - printf(" blk count\t\t= %d\n", (unsigned int)di->di_stor.block_count); + printf(" if_type\t\t= %d\n", di->di_stor.if_type); + printf(" dev\t\t= %d\n", di->di_stor.dev); + printf(" part_type\t\t= %d\n", di->di_stor.part_type); + printf(" target\t\t= %d\n", di->di_stor.target); + printf(" lun\t\t= %d\n", di->di_stor.lun); + printf(" device type\t\t= %d\n", di->di_stor.type); + printf(" removable\t\t= %d\n", di->di_stor.removable); + printf(" lba48\t\t= %d\n", di->di_stor.lba48); + printf(" blk size\t\t= %d\n", (unsigned int)di->di_stor.blksz); + printf(" blk count\t\t= %d\n", (unsigned int)di->di_stor.lba); + printf(" vendor\t\t= %s\n", di->di_stor.vendor); + printf(" product\t\t= %s\n", di->di_stor.product); + printf(" revision\t\t= %s\n", di->di_stor.revision); } } diff --git a/include/api_public.h b/include/api_public.h index 5940d81..245904f 100644 --- a/include/api_public.h +++ b/include/api_public.h @@ -111,12 +111,7 @@ struct sys_info { int mr_no; /* number of memory regions */ };
-#undef CONFIG_SYS_64BIT_LBA -#ifdef CONFIG_SYS_64BIT_LBA -typedef u_int64_t lbasize_t; -#else typedef unsigned long lbasize_t; -#endif typedef unsigned long lbastart_t;
#define DEV_TYP_NONE 0x0000 @@ -138,8 +133,7 @@ struct device_info {
union { struct { - lbasize_t block_count; /* no of blocks */ - unsigned long block_size; /* size of one block */ + #include "block_dev_attr.h" } storage;
struct { diff --git a/include/block_dev_attr.h b/include/block_dev_attr.h new file mode 100644 index 0000000..07a76d8 --- /dev/null +++ b/include/block_dev_attr.h @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +/* + * These are attributes for struct like block_dev_desc. Include this header + * inside a struct definition to populate that struct with these attributes. + */ + +int if_type; /* type of the interface */ +int dev; /* device number */ +unsigned char part_type; /* partition type */ +unsigned char target; /* target SCSI ID */ +unsigned char lun; /* target LUN */ +unsigned char type; /* device type */ +unsigned char removable; /* removable device */ +unsigned char lba48; /* device can use 48bit addr (ATA/ATAPI v7) */ +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 */ +char revision[8+1]; /* firmware revision */ diff --git a/include/part.h b/include/part.h index be0a22e..2d2b599 100644 --- a/include/part.h +++ b/include/part.h @@ -26,19 +26,9 @@ #include <ide.h>
typedef struct block_dev_desc { - int if_type; /* type of the interface */ - int dev; /* device number */ - unsigned char part_type; /* partition type */ - unsigned char target; /* target SCSI ID */ - unsigned char lun; /* target LUN */ - unsigned char type; /* device type */ - unsigned char removable; /* removable device */ - unsigned char lba48; /* device can use 48bit addr (ATA/ATAPI v7) */ - 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 */ - char revision[8+1]; /* firmware revision */ + + #include "block_dev_attr.h" + unsigned long (*block_read)(int dev, unsigned long start, lbaint_t blkcnt,
On Friday 21 October 2011 02:31:48 Che-Liang Chiou wrote:
struct device_info in api_public.h defined its own subset of attributes of block_dev_desc, which limits the capability of external apps.
This patch let struct device_info and block_dev_desc share the same set of attributes so that an external app has equal amount of information of block device compared to U-Boot.
The export.h and _export.h have somewhat addressed the same issue. That is, sharing declarations between U-Boot and external apps.
NAK: any changes to api_public.h that is not backwards ABI compatible must increment API_SIG_VERSION. you're changing the exported struct which is an ABI breakage. -mike
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 ---
Fix a minor checkpatch violation.
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(-)
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 */
Hi,
struct device_info in api_public.h defined its own subset of attributes of block_dev_desc, which limits the capability of external apps.
This patch set let external apps access the same set of block device attributes as U-Boot.
Generally speaking, we are intentionally limiting our API to external applications. It should be easier to extend U-Boot itself (and everybody will profit from the new code under a free license) rather than writing proprietary applications.
Can you pleas tell us what the intention of the extension is? We surely will not accept patches gradually exposing all of the U-Boot internals through the API and paving the way for proprietary applications. So every extension has to be balanced and discussed.
Thanks Detlev
Hi Detlev,
Oops, I did not know it is intentionally to keep the external apps API as it is now.
I am working on an open source secure bootloader based on U-Boot. Mostly I wrote boot logic (by boot logic I mean prompting user and listing available devices sort of things). If you see U-Boot as a platform, you can think of my project as an application running on top of U-Boot, except that my application is statically linked with U-Boot.
The boot logic is running on ARM and x86. Through the project I have learned that certain APIs would be helpful, such as enumerating all available block device and drawing bitmaps on screen regardless of which driver (video or LCD) you are using.
It was probably a misleading finding when I searched the code base: I saw only the external apps API implemented an API for enumerating available block device. So I thought improving the external apps API was the right place to go.
Alternatively (if not go the external apps API), we could have like common/cmd_enum_block_dev.c that does what I am planning to do, and I am happy to do this. What do you think?
Regards, Che-Liang
On Sat, Oct 22, 2011 at 12:06 AM, Detlev Zundel dzu@denx.de wrote:
Hi,
struct device_info in api_public.h defined its own subset of attributes of block_dev_desc, which limits the capability of external apps.
This patch set let external apps access the same set of block device attributes as U-Boot.
Generally speaking, we are intentionally limiting our API to external applications. It should be easier to extend U-Boot itself (and everybody will profit from the new code under a free license) rather than writing proprietary applications.
Can you pleas tell us what the intention of the extension is? We surely will not accept patches gradually exposing all of the U-Boot internals through the API and paving the way for proprietary applications. So every extension has to be balanced and discussed.
Thanks Detlev
-- Insider comment on Microsoft releasing Linux Hyper-V driver code under GPLv2: "It looks like hell just froze over." -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de
Hi Che-liang,
I am working on an open source secure bootloader based on U-Boot. Mostly I wrote boot logic (by boot logic I mean prompting user and listing available devices sort of things). If you see U-Boot as a platform, you can think of my project as an application running on top of U-Boot, except that my application is statically linked with U-Boot.
Usually the (old or new) API is used to separate the application from the GPLed U-Boot codebase to allow for proprietary applications. If you plan to link your application statically to U-Boot anyway, then I think that using the (external) API is not a good way to move forward.
The boot logic is running on ARM and x86. Through the project I have learned that certain APIs would be helpful, such as enumerating all available block device and drawing bitmaps on screen regardless of which driver (video or LCD) you are using.
Yes, I think what you actually are looking for is a proper device model in U-Boot. Actually we are in dire need for something like this in the whole of U-Boots codebase and we talk about it for a very long time (see for example [1]). This would ease progress in multiple areas, where currently we hack around this. See for example the SERIAL_MULTI code base or NET_MULTI. Both are isolated solutions for the same problem. As you note, video and block devices will also profit - USB will be another instance.
It was probably a misleading finding when I searched the code base: I saw only the external apps API implemented an API for enumerating available block device. So I thought improving the external apps API was the right place to go.
Alternatively (if not go the external apps API), we could have like common/cmd_enum_block_dev.c that does what I am planning to do, and I am happy to do this. What do you think?
I think we should aim at a pretty generic device interface. Currently I would think that this also needs to fit into the "configure U-Boot through the device tree" topic. The one will profit from the other.
As far as I know, Marek (on CC) is working towards this device tree model also. Coordinating the efforts will be a good thing.
Thanks! Detlev
participants (5)
-
Che-Liang Chiou -
Che-liang Chiou
-
Detlev Zundel -
Mike Frysinger -
Wolfgang Denk