| Message ID | 20180904074948.18146-2-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | subject: fs: fat: extend FAT write operations | expand |
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. > > 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. 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_ */ >
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). Did you run Travis CI on your patch series? >> >> 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: 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_ */ >> >
On Tue, Sep 04, 2018 at 10:52:20AM +0200, 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. > > > > 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. Let's discuss in a reply to Heinrich's comment. > 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? Sure; See https://lists.denx.de/pipermail/u-boot/2018-August/338054.html In short, one macro definition in this file will break without CONFI_FS_FAT_*. Thanks, -Takahiro AKASHI > > 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_ */ > >
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_ */ > >> > >
On 09/05/2018 03:54 AM, AKASHI Takahiro wrote: >> Did you run Travis CI on your patch series? > No. Is everybody required to run Travis CI for u-boot > before any submission? > The custodian (Alex) will run Travis CI tests and if they fail you don't get your patch series merged. So it is advisable for the submitter of the patch series to already run the same tests, especially for large changes. This will require that you have a repository on Github and authorize Travis CI to access it. Then every time you push to the repository the Travis test suite will be run. As this takes several hours I have decided to use a separate repository for Travis testing only. Best regards Heinrich
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 + #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_ */
The whole content of include/fat.h is private to FAT implementation and then should be guarded with CONFIG_FS_FAT. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/fat.h | 3 +++ 1 file changed, 3 insertions(+)