| Message ID | 20220419010158.47034-4-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted |
| Commit | 2a0d1881ac10a447cc7743c79385f744eb494718 |
| Headers | show |
| Series | disk: don't compile in partition support for spl/tpl if not really necessary | expand |
On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote: > Some defconfig enables CMD_PART even if none of any partition table > types (CONFIG_*_PARTITION) are enabled. > This will lead to the size growth in SPL/TPL code since disk/part.c > will be compiled in any way. > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only > enabled when, at least, one of CONFIG_*_PARTITION is enabled. > > To make the build work (in particular, "part" command) correctly, > a few functions should be defined as void functions in case of > !CONFIG_PARTITIONS. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> I guess I wonder why we don't just make CMD_PART depend on PARTITIONS now and thus correct the few (single?) board that has this enabled without underlying partition code by removing the can't be functional cmd.
On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote: > On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote: > > > Some defconfig enables CMD_PART even if none of any partition table > > types (CONFIG_*_PARTITION) are enabled. > > This will lead to the size growth in SPL/TPL code since disk/part.c > > will be compiled in any way. > > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only > > enabled when, at least, one of CONFIG_*_PARTITION is enabled. > > > > To make the build work (in particular, "part" command) correctly, > > a few functions should be defined as void functions in case of > > !CONFIG_PARTITIONS. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > I guess I wonder why we don't just make CMD_PART depend on PARTITIONS > now and thus correct the few (single?) board that has this enabled > without underlying partition code by removing the can't be functional > cmd. Well, that is partially what I did in my RFC and I thought that you declined to accept my change. Did I misunderstand you? -Takahiro Akashi > -- > Tom
On Tue, Apr 19, 2022 at 01:11:23PM +0900, AKASHI Takahiro wrote: > On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote: > > On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote: > > > > > Some defconfig enables CMD_PART even if none of any partition table > > > types (CONFIG_*_PARTITION) are enabled. > > > This will lead to the size growth in SPL/TPL code since disk/part.c > > > will be compiled in any way. > > > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only > > > enabled when, at least, one of CONFIG_*_PARTITION is enabled. > > > > > > To make the build work (in particular, "part" command) correctly, > > > a few functions should be defined as void functions in case of > > > !CONFIG_PARTITIONS. > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > I guess I wonder why we don't just make CMD_PART depend on PARTITIONS > > now and thus correct the few (single?) board that has this enabled > > without underlying partition code by removing the can't be functional > > cmd. > > Well, that is partially what I did in my RFC and I thought > that you declined to accept my change. > Did I misunderstand you? Yes, I wasn't clear, sorry for the confusion. Just this part of the series should be replaced with making CMD_PART depend on PARTITIONS and if there really is a use case for 'part' without PARTITION support (rather than it being an unintentionally enabled feature) we can deal with it then. The rest of the series looks good to me and I'll let Heinrich comment on the EFI specific parts.
Hi Tom, On Tue, Apr 19, 2022 at 08:12:07AM -0400, Tom Rini wrote: > On Tue, Apr 19, 2022 at 01:11:23PM +0900, AKASHI Takahiro wrote: > > On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote: > > > On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote: > > > > > > > Some defconfig enables CMD_PART even if none of any partition table > > > > types (CONFIG_*_PARTITION) are enabled. > > > > This will lead to the size growth in SPL/TPL code since disk/part.c > > > > will be compiled in any way. > > > > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only > > > > enabled when, at least, one of CONFIG_*_PARTITION is enabled. > > > > > > > > To make the build work (in particular, "part" command) correctly, > > > > a few functions should be defined as void functions in case of > > > > !CONFIG_PARTITIONS. > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > I guess I wonder why we don't just make CMD_PART depend on PARTITIONS > > > now and thus correct the few (single?) board that has this enabled > > > without underlying partition code by removing the can't be functional > > > cmd. > > > > Well, that is partially what I did in my RFC and I thought > > that you declined to accept my change. > > Did I misunderstand you? > > Yes, I wasn't clear, sorry for the confusion. Just this part of the > series should be replaced with making CMD_PART depend on PARTITIONS and > if there really is a use case for 'part' without PARTITION support > (rather than it being an unintentionally enabled feature) we can deal > with it then. The rest of the series looks good to me and I'll let > Heinrich comment on the EFI specific parts. I do understand what you expect here, but, what I call, "nullified function" technique is already used in several places. For instance, take blk_get_device_part_str() function which has a nullified version of definition in include/part.h. It is used without explicit dependency on CONFIG_PARTITIONS at: cmd/zfs.c cmd/disk.c cmd/reiser.c cmd/fat.c env/ext4.c env/fat.c So I would like to propose to create another patch that you expect (and what I did in RFC) instead of removing this patch because the latter has no negative effect. If you agree, I will post it as a separate patch. -Takahiro Akashi > -- > Tom
On Wed, Apr 20, 2022 at 11:17:21AM +0900, AKASHI Takahiro wrote: > Hi Tom, > > On Tue, Apr 19, 2022 at 08:12:07AM -0400, Tom Rini wrote: > > On Tue, Apr 19, 2022 at 01:11:23PM +0900, AKASHI Takahiro wrote: > > > On Mon, Apr 18, 2022 at 11:09:38PM -0400, Tom Rini wrote: > > > > On Tue, Apr 19, 2022 at 10:01:54AM +0900, AKASHI Takahiro wrote: > > > > > > > > > Some defconfig enables CMD_PART even if none of any partition table > > > > > types (CONFIG_*_PARTITION) are enabled. > > > > > This will lead to the size growth in SPL/TPL code since disk/part.c > > > > > will be compiled in any way. > > > > > We will change disk/Kconfig later so that CONFIG_PARTITIONS is only > > > > > enabled when, at least, one of CONFIG_*_PARTITION is enabled. > > > > > > > > > > To make the build work (in particular, "part" command) correctly, > > > > > a few functions should be defined as void functions in case of > > > > > !CONFIG_PARTITIONS. > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > > > > I guess I wonder why we don't just make CMD_PART depend on PARTITIONS > > > > now and thus correct the few (single?) board that has this enabled > > > > without underlying partition code by removing the can't be functional > > > > cmd. > > > > > > Well, that is partially what I did in my RFC and I thought > > > that you declined to accept my change. > > > Did I misunderstand you? > > > > Yes, I wasn't clear, sorry for the confusion. Just this part of the > > series should be replaced with making CMD_PART depend on PARTITIONS and > > if there really is a use case for 'part' without PARTITION support > > (rather than it being an unintentionally enabled feature) we can deal > > with it then. The rest of the series looks good to me and I'll let > > Heinrich comment on the EFI specific parts. > > I do understand what you expect here, but, what I call, "nullified > function" technique is already used in several places. > For instance, take blk_get_device_part_str() function which has > a nullified version of definition in include/part.h. > It is used without explicit dependency on CONFIG_PARTITIONS at: > cmd/zfs.c > cmd/disk.c > cmd/reiser.c > cmd/fat.c > env/ext4.c > env/fat.c > > So I would like to propose to create another patch that you expect (and > what I did in RFC) instead of removing this patch because the latter > has no negative effect. > > If you agree, I will post it as a separate patch. OK, lets go with that then, thanks!
diff --git a/include/part.h b/include/part.h index 9975fad97121..5f47a76bc19b 100644 --- a/include/part.h +++ b/include/part.h @@ -276,6 +276,15 @@ static inline int blk_get_device_part_str(const char *ifname, struct disk_partition *info, int allow_whole_dev) { *dev_desc = NULL; return -1; } +static inline int part_get_info_by_name_type(struct blk_desc *dev_desc, + const char *name, + struct disk_partition *info, + int part_type) +{ return -ENOENT; } +static inline int part_get_info_by_name(struct blk_desc *dev_desc, + const char *name, + struct disk_partition *info) +{ return -ENOENT; } static inline int part_get_info_by_dev_and_name_or_num(const char *dev_iface, const char *dev_part_str,
Some defconfig enables CMD_PART even if none of any partition table types (CONFIG_*_PARTITION) are enabled. This will lead to the size growth in SPL/TPL code since disk/part.c will be compiled in any way. We will change disk/Kconfig later so that CONFIG_PARTITIONS is only enabled when, at least, one of CONFIG_*_PARTITION is enabled. To make the build work (in particular, "part" command) correctly, a few functions should be defined as void functions in case of !CONFIG_PARTITIONS. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/part.h | 9 +++++++++ 1 file changed, 9 insertions(+)