From mboxrd@z Thu Jan  1 00:00:00 1970
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Wed, 5 Sep 2018 10:54:47 +0900
Subject: [U-Boot] [PATCH v2 01/23] fs: fat: guard the content of
	include/fat.h
In-Reply-To: <67cc9114-18bb-6c7e-37c0-f75170ccdd8b@gmx.de>
References: <20180904074948.18146-1-takahiro.akashi@linaro.org>
 <20180904074948.18146-2-takahiro.akashi@linaro.org>
 <71b5287f-2a0d-c639-5b66-3ae0e21ab159@suse.de>
 <67cc9114-18bb-6c7e-37c0-f75170ccdd8b@gmx.de>
Message-ID: <20180905015446.GC18483@linaro.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Heinrich, Alex,

On Tue, Sep 04, 2018 at 12:46:58PM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 09/04/2018 10:52 AM, Alexander Graf wrote:
> > 
> > 
> > On 04.09.18 09:49, AKASHI Takahiro wrote:
> >> The whole content of include/fat.h is private to FAT implementation
> >> and then should be guarded with CONFIG_FS_FAT.
> 
> Hello Takahiro,
> 
> doesn't this imply that building common/spl/spl_sata.c without FAT will
> fail for CONFIG_SPL_SATA_SUPPORT=y (e.g. cm_t54_defconfig with FAT
> disabled).

Yes, you're right, but this is a matter of cm_t54_defconfig.
spl_sta.c uses spl_load_image_fat[_os]() which are implemented
in spl_fat.c which replies on CONFIG_SPL_FAT_SUPPORT, which
will in turn select CONFIG_FS_FAT.
The problem is that cm_t54_defconfig doesn't enable
CONFIG_SPL_FAT_SUPPORT.

To fix this issue, we should add "depends on SPL_FAT_SUPPORT"
to SPL_SATA_SUPPORT (and SPL_USB_SUPPORT) as well.

> Did you run Travis CI on your patch series?

No. Is everybody required to run Travis CI for u-boot
before any submission?

> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> ---
> >>  include/fat.h | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/include/fat.h b/include/fat.h
> >> index 09e142368585..c02839dcb040 100644
> >> --- a/include/fat.h
> >> +++ b/include/fat.h
> >> @@ -9,6 +9,8 @@
> >>  #ifndef _FAT_H_
> >>  #define _FAT_H_
> >>  
> >> +#ifdef CONFIG_FS_FAT
> > 
> > Isn't this missing an include of at least common.h to actually propagate
> > the config define?
> > 
> > Also, you want to use #if CONFIG_IS_ENABLED(...) here to guard against
> > SPL builds too.
> 
> Probably not:

As I'm saying, CONFIG_SPL_FAT_SUPPORT selects CONFIG_FS_FAT
and so either "ifdef CONFIG_FS_FAT" or "if  CONFIG_IS_ENABLED(FS_FAT)" will
turn into True although CONFIG_SPL_FS_FAT actually doesn't exist.

Therefore, I don't think we need a change Alex suggested.

Thanks,
-Takahiro AKASHI

> common/spl/spl_fat.c:14:#include <fat.h>
> 
> Best regards
> 
> Heinrich Schuchardt
> 
> > 
> > However, I don't fully grasp why you need this patch. Please describe in
> > your commit message what the incentive is for creating it. What errors
> > are you trying to protect against?
> > 
> > 
> > Alex
> > 
> >> +
> >>  #include <asm/byteorder.h>
> >>  #include <fs.h>
> >>  
> >> @@ -202,4 +204,5 @@ int fat_opendir(const char *filename, struct fs_dir_stream **dirsp);
> >>  int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp);
> >>  void fat_closedir(struct fs_dir_stream *dirs);
> >>  void fat_close(void);
> >> +#endif /* CONFIG_FS_FAT */
> >>  #endif /* _FAT_H_ */
> >>
> >