[PATCH] Staging: android: Remove unneeded definition in header file

[PATCH] Staging: android: Remove unneeded definition in header file

[Shraddha Barke]

These struct are globally defined in source file but they have been
defined as static in .h file. Remove definition in header file as
it is not needed. Also fix the compiler warning.

Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
---
 drivers/staging/android/sw_sync.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/staging/android/sw_sync.h b/drivers/staging/android/sw_sync.h
index c87ae9e..a671595 100644
--- a/drivers/staging/android/sw_sync.h
+++ b/drivers/staging/android/sw_sync.h
@@ -39,21 +39,6 @@ struct sw_sync_timeline *sw_sync_timeline_create(const char *name);
 void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc);
 
 struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj, u32 value);
-#else
-static inline struct sw_sync_timeline *sw_sync_timeline_create(const char *name)
-{
-	return NULL;
-}
-
-static inline void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc)
-{
-}
-
-static inline struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj,
-						u32 value)
-{
-	return NULL;
-}
 #endif /* IS_ENABLED(CONFIG_SW_SYNC) */
 
 #endif /* _LINUX_SW_SYNC_H */
-- 
2.1.4

Re: [Outreachy kernel] [PATCH] Staging: android: Remove unneeded definition in header file

[Greg KH]

On Thu, Oct 01, 2015 at 09:49:26AM +0530, Shraddha Barke wrote:
> These struct are globally defined in source file but they have been
> defined as static in .h file. Remove definition in header file as
> it is not needed. Also fix the compiler warning.
> 
> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> ---
>  drivers/staging/android/sw_sync.h | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/staging/android/sw_sync.h b/drivers/staging/android/sw_sync.h
> index c87ae9e..a671595 100644
> --- a/drivers/staging/android/sw_sync.h
> +++ b/drivers/staging/android/sw_sync.h
> @@ -39,21 +39,6 @@ struct sw_sync_timeline *sw_sync_timeline_create(const char *name);
>  void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc);
>  
>  struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj, u32 value);
> -#else
> -static inline struct sw_sync_timeline *sw_sync_timeline_create(const char *name)
> -{
> -	return NULL;
> -}
> -
> -static inline void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc)
> -{
> -}
> -
> -static inline struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj,
> -						u32 value)
> -{
> -	return NULL;
> -}
>  #endif /* IS_ENABLED(CONFIG_SW_SYNC) */
>  
>  #endif /* _LINUX_SW_SYNC_H */

You just broke the build for systems that don't define CONFIG_SW_SYNC
yet still call these functions :(

Yes, it's tricky as there are no in-kernel users of these functions,
that's the joy of the android code, external, out-of-tree code depends
on the in-kernel code right now due to not everything being merged yet.

So, sorry, I can't take this patch.

greg k-h

Re: [Outreachy kernel] [PATCH] Staging: android: Remove unneeded definition in header file

[Sudip Mukherjee]

On Thu, Oct 01, 2015 at 07:56:24AM +0200, Greg KH wrote:
> On Thu, Oct 01, 2015 at 09:49:26AM +0530, Shraddha Barke wrote:
> > These struct are globally defined in source file but they have been
> > defined as static in .h file. Remove definition in header file as
> > it is not needed. Also fix the compiler warning.
> > 
> > Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> > ---
> >  drivers/staging/android/sw_sync.h | 15 ---------------
> >  1 file changed, 15 deletions(-)
> > 
> > diff --git a/drivers/staging/android/sw_sync.h b/drivers/staging/android/sw_sync.h
> > index c87ae9e..a671595 100644
> > --- a/drivers/staging/android/sw_sync.h
> > +++ b/drivers/staging/android/sw_sync.h
> > @@ -39,21 +39,6 @@ struct sw_sync_timeline *sw_sync_timeline_create(const char *name);
> >  void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc);
> >  
> >  struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj, u32 value);
> > -#else
> > -static inline struct sw_sync_timeline *sw_sync_timeline_create(const char *name)
> > -{
> > -	return NULL;
> > -}
> > -
> > -static inline void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc)
> > -{
> > -}
> > -
> > -static inline struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj,
> > -						u32 value)
> > -{
> > -	return NULL;
> > -}
> >  #endif /* IS_ENABLED(CONFIG_SW_SYNC) */
> >  
> >  #endif /* _LINUX_SW_SYNC_H */
> 
> You just broke the build for systems that don't define CONFIG_SW_SYNC
> yet still call these functions :(

I had the same doubt so i tested it with CONFIG_SW_SYNC disabled in my
.config, but the build didn't break.
Is it because the only use of sw_sync_timeline_inc is from sw_sync.c
which will build only if CONFIG_SW_SYNC is defined?

regards
sudip

Re: [Outreachy kernel] [PATCH] Staging: android: Remove unneeded definition in header file

[Greg KH]

On Thu, Oct 01, 2015 at 11:44:00AM +0530, Sudip Mukherjee wrote:
> On Thu, Oct 01, 2015 at 07:56:24AM +0200, Greg KH wrote:
> > On Thu, Oct 01, 2015 at 09:49:26AM +0530, Shraddha Barke wrote:
> > > These struct are globally defined in source file but they have been
> > > defined as static in .h file. Remove definition in header file as
> > > it is not needed. Also fix the compiler warning.
> > > 
> > > Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> > > ---
> > >  drivers/staging/android/sw_sync.h | 15 ---------------
> > >  1 file changed, 15 deletions(-)
> > > 
> > > diff --git a/drivers/staging/android/sw_sync.h b/drivers/staging/android/sw_sync.h
> > > index c87ae9e..a671595 100644
> > > --- a/drivers/staging/android/sw_sync.h
> > > +++ b/drivers/staging/android/sw_sync.h
> > > @@ -39,21 +39,6 @@ struct sw_sync_timeline *sw_sync_timeline_create(const char *name);
> > >  void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc);
> > >  
> > >  struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj, u32 value);
> > > -#else
> > > -static inline struct sw_sync_timeline *sw_sync_timeline_create(const char *name)
> > > -{
> > > -	return NULL;
> > > -}
> > > -
> > > -static inline void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc)
> > > -{
> > > -}
> > > -
> > > -static inline struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj,
> > > -						u32 value)
> > > -{
> > > -	return NULL;
> > > -}
> > >  #endif /* IS_ENABLED(CONFIG_SW_SYNC) */
> > >  
> > >  #endif /* _LINUX_SW_SYNC_H */
> > 
> > You just broke the build for systems that don't define CONFIG_SW_SYNC
> > yet still call these functions :(
> 
> I had the same doubt so i tested it with CONFIG_SW_SYNC disabled in my
> .config, but the build didn't break.

That is because this code is for out-of-tree code :(

> Is it because the only use of sw_sync_timeline_inc is from sw_sync.c
> which will build only if CONFIG_SW_SYNC is defined?

Yes, but other files might use those functions and they need to still
build if the config option is not enabled.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] Staging: android: Remove unneeded definition in header file

[Julia Lawall]

> Yes, but other files might use those functions and they need to still
> build if the config option is not enabled.

Are there some drivers in particular for which one should watch out for
this issue?

thanks,
julia

Re: [Outreachy kernel] [PATCH] Staging: android: Remove unneeded definition in header file

[Greg KH]

On Thu, Oct 01, 2015 at 10:04:38AM +0200, Julia Lawall wrote:
> > Yes, but other files might use those functions and they need to still
> > build if the config option is not enabled.
> 
> Are there some drivers in particular for which one should watch out for
> this issue?

drivers/staging/android/ should be the only stuff that has this issue.
I'd recommend everyone staying away from drivers/staging/android/ unless
you are an Andoid kernel developer, and even those people know to keep
away from it :)

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] Staging: android: Remove unneeded definition in header file

[Shraddha Barke]

On Thu, 1 Oct 2015, Greg KH wrote:

> On Thu, Oct 01, 2015 at 09:49:26AM +0530, Shraddha Barke wrote:
>> These struct are globally defined in source file but they have been
>> defined as static in .h file. Remove definition in header file as
>> it is not needed. Also fix the compiler warning.
>>
>> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
>> ---
>>  drivers/staging/android/sw_sync.h | 15 ---------------
>>  1 file changed, 15 deletions(-)
>>
>> diff --git a/drivers/staging/android/sw_sync.h b/drivers/staging/android/sw_sync.h
>> index c87ae9e..a671595 100644
>> --- a/drivers/staging/android/sw_sync.h
>> +++ b/drivers/staging/android/sw_sync.h
>> @@ -39,21 +39,6 @@ struct sw_sync_timeline *sw_sync_timeline_create(const char *name);
>>  void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc);
>>
>>  struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj, u32 value);
>> -#else
>> -static inline struct sw_sync_timeline *sw_sync_timeline_create(const char *name)
>> -{
>> -	return NULL;
>> -}
>> -
>> -static inline void sw_sync_timeline_inc(struct sw_sync_timeline *obj, u32 inc)
>> -{
>> -}
>> -
>> -static inline struct sync_pt *sw_sync_pt_create(struct sw_sync_timeline *obj,
>> -						u32 value)
>> -{
>> -	return NULL;
>> -}
>>  #endif /* IS_ENABLED(CONFIG_SW_SYNC) */
>>
>>  #endif /* _LINUX_SW_SYNC_H */
>
> You just broke the build for systems that don't define CONFIG_SW_SYNC
> yet still call these functions :(
>
> Yes, it's tricky as there are no in-kernel users of these functions,
> that's the joy of the android code, external, out-of-tree code depends
> on the in-kernel code right now due to not everything being merged yet.

I'm sorry. I had build tested it by disabling the CONFIG_SW_SYNC option
before sending but like you said since it's out of kernel code, couldn't
figure out that it's wrong. Is there a way to differentiate such cases?

Thanks,

Shraddha

>
> So, sorry, I can't take this patch.
>
> greg k-h
>