[PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[Rashika Kheria]

Include appropriate header file include/linux/usb/otg.h in core/hcd.c
because function usb_bus_start_enum() has its prototype declaration in
include/linux/usb/otg.h.

This eliminates the following warning in core/hcd.c:
drivers/usb/core/hcd.c:2295:5: warning: no previous prototype for ‘usb_bus_start_enum’ [-Wmissing-prototypes]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 drivers/usb/core/hcd.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 6bffb8c..077be7c 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -44,6 +44,7 @@
 
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/otg.h>
 
 #include "usb.h"
 
-- 
1.7.9.5

[PATCH 2/7] drivers: usb: Include appropriate header file in configfs.c

[Rashika Kheria]

Include appropriate header file drivers/usb/gadget/configfs.h in
gadget/configfs.c because function unregister_gadget_item() has its
prototype declaration in gadget/configfs.h.

This eliminates the following warning in gadget/configfs.c:
drivers/usb/gadget/configfs.c:994:6: warning: no previous prototype for ‘unregister_gadget_item’ [-Wmissing-prototypes]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 drivers/usb/gadget/configfs.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 2588511..a117ec2 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -4,6 +4,7 @@
 #include <linux/device.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/gadget_configfs.h>
+#include "configfs.h"
 
 int check_user_usb_string(const char *name,
 		struct usb_gadget_strings *stringtab_dev)
-- 
1.7.9.5

[PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Rashika Kheria]

Include header file include/linux/usb.h in include/linux/usb/hcd.h
because structures usb_device, usb_host_config and usb_interface have
their definitions in include/linux/usb.h.

This eliminates the following warning in include/linux/usb/hcd.h:
include/linux/usb/hcd.h:311:44: warning: ‘struct usb_device’ declared inside parameter list [enabled by default]
include/linux/usb/hcd.h:412:10: warning: ‘struct usb_host_config’ declared inside parameter list [enabled by default]
include/linux/usb/hcd.h:614:9: warning: ‘struct usb_interface’ declared inside parameter list [enabled by default]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 include/linux/usb/hcd.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index b8aba19..f312ce4 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -23,6 +23,7 @@
 
 #include <linux/rwsem.h>
 #include <linux/interrupt.h>
+#include <linux/usb.h>
 
 #define MAX_TOPO_LEVEL		6
 
-- 
1.7.9.5

[PATCH 4/7] drivers: usb: Include appropriate header file in pci-quirks.c

[Rashika Kheria]

Include header file include/linux/usb/hcd.h in host/pci-quirks.c because
function usb_hcd_amd_remote_wakeup_quirk() has its prototype declaration
in include/linux/usb/hcd.h.

This eliminates the following warning in host/pci-quirks.c:
drivers/usb/host/pci-quirks.c:253:5: warning: no previous prototype for ‘usb_hcd_amd_remote_wakeup_quirk’ [-Wmissing-prototypes]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 drivers/usb/host/pci-quirks.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index dfbdd3a..4574953 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -17,6 +17,7 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/usb/hcd.h>
 #include "pci-quirks.h"
 #include "xhci-ext-caps.h"
 
-- 
1.7.9.5

[PATCH 5/7] drivers: usb: Mark function as static in usbsevseg.c

[Rashika Kheria]

Mark function my_memlen() as static in misc/usbsevseg.c because it is
not used outside this file.

This eliminates the following warning in misc/usbsevseg.c:
drivers/usb/misc/usbsevseg.c:60:15: warning: no previous prototype for ‘my_memlen’ [-Wmissing-prototypes]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 drivers/usb/misc/usbsevseg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index b2d82b9..0a87d3e 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -57,7 +57,7 @@ struct usb_sevsegdev {
  * if str commands are used, we would assume the end of string
  * so mem commands are used.
  */
-inline size_t my_memlen(const char *buf, size_t count)
+static inline size_t my_memlen(const char *buf, size_t count)
 {
 	if (count > 0 && buf[count-1] == '\n')
 		return count - 1;
-- 
1.7.9.5

[PATCH 6/7] drivers: usb: Mark function as static in metro-usb.c

[Rashika Kheria]

Mark function metrousb_is_unidirectional_mode() in serial/metro-usb.c
because it is not used outside this file.

This eliminates the following warning in serial/metro-usb.c:
drivers/usb/serial/metro-usb.c:57:12: warning: no previous prototype for ‘metrousb_is_unidirectional_mode’ [-Wmissing-prototypes]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 drivers/usb/serial/metro-usb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/metro-usb.c b/drivers/usb/serial/metro-usb.c
index 40ccf6e..2b648c4 100644
--- a/drivers/usb/serial/metro-usb.c
+++ b/drivers/usb/serial/metro-usb.c
@@ -54,7 +54,7 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UNI_CMD_OPEN	0x80
 #define UNI_CMD_CLOSE	0xFF
 
-inline int metrousb_is_unidirectional_mode(struct usb_serial_port *port)
+static inline int metrousb_is_unidirectional_mode(struct usb_serial_port *port)
 {
 	__u16 product_id = le16_to_cpu(
 		port->serial->dev->descriptor.idProduct);
-- 
1.7.9.5

[PATCH 7/7] drivers: usb: Include appropriate header file in phy-am335x-control.c

[Rashika Kheria]

Include header file drivers/usb/phy/am35x-phy-control.h in
phy/phy-am335x-control.c because function am335x_get_phy_control() has
its prototype declaration in drivers/usb/phy/am35x-phy-control.h.

Also, remove definition of structure phy_control because it is already
defined in the included header.

This eliminates the following warning in phy/phy-am335x-control.c:
drivers/usb/phy/phy-am335x-control.c:114:21: warning: no previous prototype for ‘am335x_get_phy_control’ [-Wmissing-prototypes]

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 drivers/usb/phy/phy-am335x-control.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/usb/phy/phy-am335x-control.c b/drivers/usb/phy/phy-am335x-control.c
index 634f49a..d75196a 100644
--- a/drivers/usb/phy/phy-am335x-control.c
+++ b/drivers/usb/phy/phy-am335x-control.c
@@ -3,11 +3,7 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <linux/io.h>
-
-struct phy_control {
-	void (*phy_power)(struct phy_control *phy_ctrl, u32 id, bool on);
-	void (*phy_wkup)(struct phy_control *phy_ctrl, u32 id, bool on);
-};
+#include "am35x-phy-control.h"
 
 struct am335x_control_usb {
 	struct device *dev;
-- 
1.7.9.5

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Alan Stern]

On Thu, 19 Dec 2013, Rashika Kheria wrote:

> Include header file include/linux/usb.h in include/linux/usb/hcd.h
> because structures usb_device, usb_host_config and usb_interface have
> their definitions in include/linux/usb.h.
> 
> This eliminates the following warning in include/linux/usb/hcd.h:
> include/linux/usb/hcd.h:311:44: warning: ‘struct usb_device’ declared inside parameter list [enabled by default]
> include/linux/usb/hcd.h:412:10: warning: ‘struct usb_host_config’ declared inside parameter list [enabled by default]
> include/linux/usb/hcd.h:614:9: warning: ‘struct usb_interface’ declared inside parameter list [enabled by default]

Where does this problem show up?

Any file that include linux/usb/hcd.h should include linux/usb.h first.  
IMO it would be better to fix the source files that don't do the 
includes properly.

Of course, people have varying opinions on this issue.  As far as I 
know, there is no fixed policy in the kernel about nested includes.

Alan Stern

Re: [PATCH 4/7] drivers: usb: Include appropriate header file in pci-quirks.c

[Alan Stern]

On Thu, 19 Dec 2013, Rashika Kheria wrote:

> Include header file include/linux/usb/hcd.h in host/pci-quirks.c because
> function usb_hcd_amd_remote_wakeup_quirk() has its prototype declaration
> in include/linux/usb/hcd.h.
> 
> This eliminates the following warning in host/pci-quirks.c:
> drivers/usb/host/pci-quirks.c:253:5: warning: no previous prototype for ‘usb_hcd_amd_remote_wakeup_quirk’ [-Wmissing-prototypes]

Since when does the compiler complain about functions with no 
prototype?

In any case, I think it would make more sense to move the prototype 
declaration into drivers/usb/host/pci-quirks.h, and have 
drivers/usb/core/hcd-pci.c include that file.  After all, it's 
perfectly reasonable for hcd-pci to want to know about the quirks of 
various PCI-based controllers, but it's not reasonable for pci-quirks.c 
to need to know about the details of HCDs.

Alan Stern

Re: [PATCH 4/7] drivers: usb: Include appropriate header file in pci-quirks.c

[josh]

On Thu, Dec 19, 2013 at 10:51:50AM -0500, Alan Stern wrote:
> On Thu, 19 Dec 2013, Rashika Kheria wrote:
> 
> > Include header file include/linux/usb/hcd.h in host/pci-quirks.c because
> > function usb_hcd_amd_remote_wakeup_quirk() has its prototype declaration
> > in include/linux/usb/hcd.h.
> > 
> > This eliminates the following warning in host/pci-quirks.c:
> > drivers/usb/host/pci-quirks.c:253:5: warning: no previous prototype for ‘usb_hcd_amd_remote_wakeup_quirk’ [-Wmissing-prototypes]
> 
> Since when does the compiler complain about functions with no 
> prototype?

Sparse has warned about this for years, and GCC warns about it (for
functions only, not data) when given -Wmissing-prototypes, which it does
with "make W=1".  The intent is to call attention to functions that may
need to be marked static or removed, and to require inclusion of an
appropriate header for non-static functions, which then allows the
compiler to ensure that the prototype matches the definition.

> In any case, I think it would make more sense to move the prototype 
> declaration into drivers/usb/host/pci-quirks.h, and have 
> drivers/usb/core/hcd-pci.c include that file.  After all, it's 
> perfectly reasonable for hcd-pci to want to know about the quirks of 
> various PCI-based controllers, but it's not reasonable for pci-quirks.c 
> to need to know about the details of HCDs.

Sounds reasonable.

- Josh Triplett

RE: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 1083 bytes --]

> From: Rashika Kheria
> Include header file include/linux/usb.h in include/linux/usb/hcd.h
> because structures usb_device, usb_host_config and usb_interface have
> their definitions in include/linux/usb.h.
> 
> This eliminates the following warning in include/linux/usb/hcd.h:
> include/linux/usb/hcd.h:311:44: warning: ‘struct usb_device’ declared inside parameter list [enabled
> by default]
> include/linux/usb/hcd.h:412:10: warning: ‘struct usb_host_config’ declared inside parameter list
> [enabled by default]
> include/linux/usb/hcd.h:614:9: warning: ‘struct usb_interface’ declared inside parameter list [enabled
> by default]

All it is necessary to do is add a declaration of the struct before the function definition.
There is no need to include the definition of the structure.
It is a shame that gcc doesn't defer this warning to any call site
(where an incorrect type would get passed).

	David
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 03:36:00PM +0530, Rashika Kheria wrote:
> Include appropriate header file include/linux/usb/otg.h in core/hcd.c
> because function usb_bus_start_enum() has its prototype declaration in
> include/linux/usb/otg.h.
> 
> This eliminates the following warning in core/hcd.c:
> drivers/usb/core/hcd.c:2295:5: warning: no previous prototype for ‘usb_bus_start_enum’ [-Wmissing-prototypes]

What is generating this warning?  I don't see it here on my machines,
nor do I see any of the other warnings you have fixed in this series.
How do I duplicate them?

thanks,

greg k-h

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[josh]

On Thu, Dec 19, 2013 at 10:45:42AM -0500, Alan Stern wrote:
> On Thu, 19 Dec 2013, Rashika Kheria wrote:
> 
> > Include header file include/linux/usb.h in include/linux/usb/hcd.h
> > because structures usb_device, usb_host_config and usb_interface have
> > their definitions in include/linux/usb.h.
> > 
> > This eliminates the following warning in include/linux/usb/hcd.h:
> > include/linux/usb/hcd.h:311:44: warning: ‘struct usb_device’ declared inside parameter list [enabled by default]
> > include/linux/usb/hcd.h:412:10: warning: ‘struct usb_host_config’ declared inside parameter list [enabled by default]
> > include/linux/usb/hcd.h:614:9: warning: ‘struct usb_interface’ declared inside parameter list [enabled by default]
> 
> Where does this problem show up?
> 
> Any file that include linux/usb/hcd.h should include linux/usb.h first.  
> IMO it would be better to fix the source files that don't do the 
> includes properly.
> 
> Of course, people have varying opinions on this issue.  As far as I 
> know, there is no fixed policy in the kernel about nested includes.

True.  I personally prefer the policy of making all headers
self-contained, and then only including headers that define things used
in the source file.  That has the advantage of not including any
unnecessary headers if the dependencies shrink, and not requiring
changes to multiple source files if the dependencies grow.

Any particular objection to making the headers self-contained?

- Josh Triplett

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Alan Stern]

On Thu, 19 Dec 2013, Sergei Shtylyov wrote:

> Hello.
> 
> On 12/19/2013 06:45 PM, Alan Stern wrote:
> 
> >> Include header file include/linux/usb.h in include/linux/usb/hcd.h
> >> because structures usb_device, usb_host_config and usb_interface have
> >> their definitions in include/linux/usb.h.
> 
> >> This eliminates the following warning in include/linux/usb/hcd.h:
> >> include/linux/usb/hcd.h:311:44: warning: ‘struct usb_device’ declared inside parameter list [enabled by default]
> >> include/linux/usb/hcd.h:412:10: warning: ‘struct usb_host_config’ declared inside parameter list [enabled by default]
> >> include/linux/usb/hcd.h:614:9: warning: ‘struct usb_interface’ declared inside parameter list [enabled by default]
> 
>     Rashika, would it be enough to forward-declare these structures ISO 
> #include'ing the whole header?

I agree, that would fix the problem.

> > Where does this problem show up?
> >
> > Any file that include linux/usb/hcd.h should include linux/usb.h first.
> > IMO it would be better to fix the source files that don't do the
> > includes properly.
> 
>     Yeah, let's fix the consequency instead of the cause. :-)

The _real_ cause is that the Linux source code is extremely
complicated, and it is remarkably difficult to insure that all header
files have no unsatisfied dependencies.  How do you suggest fixing
_that_?

For example, suppose A.c includes B.h, and B.h includes C.h, and C.h
defines struct foo.  Then A.c can use struct foo freely without
including C.h directly (and this sort of thing happens quite a lot in
the kernel source).  But consider what happens when B.h is changed so
that it no longer includes C.h.

> > Of course, people have varying opinions on this issue.  As far as I
> > know, there is no fixed policy in the kernel about nested includes.
> 
>     So far, I've only encountered the dubious policy of satisfying header's 
> dependencies in the files that include them is the USB tree.

Have you looked in any other places?  For that matter, how do you know
that the USB tree has such a policy?  Is it documented anywhere?

Alan Stern

Re: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[Rashika Kheria]

On Thu, Dec 19, 2013 at 10:06 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Dec 19, 2013 at 03:36:00PM +0530, Rashika Kheria wrote:
>> Include appropriate header file include/linux/usb/otg.h in core/hcd.c
>> because function usb_bus_start_enum() has its prototype declaration in
>> include/linux/usb/otg.h.
>>
>> This eliminates the following warning in core/hcd.c:
>> drivers/usb/core/hcd.c:2295:5: warning: no previous prototype for ‘usb_bus_start_enum’ [-Wmissing-prototypes]
>
> What is generating this warning?  I don't see it here on my machines,
> nor do I see any of the other warnings you have fixed in this series.
> How do I duplicate them?
>
> thanks,
>
> greg k-h

Hi Greg,

These warning are non-default GCC warnings. These can be seen either
by adding W=1 while running make (i.e. make W=1) or adding
-Wmissing-prototypes in KBUILD_CFLAGS in the toplevel Makefile.

Thanks,
-- 
Rashika Kheria
B.Tech CSE
IIIT Hyderabad

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Alan Stern]

On Thu, 19 Dec 2013 josh@joshtriplett.org wrote:

> > Of course, people have varying opinions on this issue.  As far as I 
> > know, there is no fixed policy in the kernel about nested includes.
> 
> True.  I personally prefer the policy of making all headers
> self-contained, and then only including headers that define things used
> in the source file.  That has the advantage of not including any
> unnecessary headers if the dependencies shrink, and not requiring
> changes to multiple source files if the dependencies grow.
> 
> Any particular objection to making the headers self-contained?

I guess it depends on what you mean by "self-contained".  The only 
reasonable definition I can think of at the moment is that you don't 
get any errors or warnings when you compile the .h file by itself.

But what use is that in practice?  Nobody ever compiles .h files by
themselves.

For that matter, how can you tell that you are including only headers 
that define things used in the source file?  Remove each #include line, 
one at a time, and see if you then get an error?  Do you do this after 
each change to the source file to make sure it remains true over time?

My point is that the C language design and compiler infrastructure make 
it virtually impossible to enforce any fixed policy.

Alan Stern

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[josh]

On Thu, Dec 19, 2013 at 11:48:15AM -0500, Alan Stern wrote:
> On Thu, 19 Dec 2013 josh@joshtriplett.org wrote:
> 
> > > Of course, people have varying opinions on this issue.  As far as I 
> > > know, there is no fixed policy in the kernel about nested includes.
> > 
> > True.  I personally prefer the policy of making all headers
> > self-contained, and then only including headers that define things used
> > in the source file.  That has the advantage of not including any
> > unnecessary headers if the dependencies shrink, and not requiring
> > changes to multiple source files if the dependencies grow.
> > 
> > Any particular objection to making the headers self-contained?
> 
> I guess it depends on what you mean by "self-contained".  The only 
> reasonable definition I can think of at the moment is that you don't 
> get any errors or warnings when you compile the .h file by itself.

Or, to look at it another way, you can #include the .h file in a .c file
without any other .h file, and successfully compile the .c file and use
everything defined by the .h file.

> For that matter, how can you tell that you are including only headers 
> that define things used in the source file?  Remove each #include line, 
> one at a time, and see if you then get an error?  Do you do this after 
> each change to the source file to make sure it remains true over time?
>
> My point is that the C language design and compiler infrastructure make 
> it virtually impossible to enforce any fixed policy.

And that leaves aside all the preprocessor symbols that might change
what a header defines.  I'd argue for a best-effort policy, together
with fixing headers whenever someone notices that they're *not*
self-contained (in other words, they include a .h file to get a
definition they need, and get a compile error).

- Josh Triplett

Re: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 10:11:45PM +0530, Rashika Kheria wrote:
> On Thu, Dec 19, 2013 at 10:06 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Dec 19, 2013 at 03:36:00PM +0530, Rashika Kheria wrote:
> >> Include appropriate header file include/linux/usb/otg.h in core/hcd.c
> >> because function usb_bus_start_enum() has its prototype declaration in
> >> include/linux/usb/otg.h.
> >>
> >> This eliminates the following warning in core/hcd.c:
> >> drivers/usb/core/hcd.c:2295:5: warning: no previous prototype for ‘usb_bus_start_enum’ [-Wmissing-prototypes]
> >
> > What is generating this warning?  I don't see it here on my machines,
> > nor do I see any of the other warnings you have fixed in this series.
> > How do I duplicate them?
> >
> > thanks,
> >
> > greg k-h
> 
> Hi Greg,
> 
> These warning are non-default GCC warnings. These can be seen either
> by adding W=1 while running make (i.e. make W=1) or adding
> -Wmissing-prototypes in KBUILD_CFLAGS in the toplevel Makefile.

By default, we don't care about 'W=1' warnings, as no one sees them, and
they don't matter.

Now if you were to see these issues with 'sparse' or just a "normal"
build, then that might be worth fixing up.

greg k-h

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Sergei Shtylyov]

Hello.

On 12/19/2013 06:45 PM, Alan Stern wrote:

>> Include header file include/linux/usb.h in include/linux/usb/hcd.h
>> because structures usb_device, usb_host_config and usb_interface have
>> their definitions in include/linux/usb.h.

>> This eliminates the following warning in include/linux/usb/hcd.h:
>> include/linux/usb/hcd.h:311:44: warning: ‘struct usb_device’ declared inside parameter list [enabled by default]
>> include/linux/usb/hcd.h:412:10: warning: ‘struct usb_host_config’ declared inside parameter list [enabled by default]
>> include/linux/usb/hcd.h:614:9: warning: ‘struct usb_interface’ declared inside parameter list [enabled by default]

    Rashika, would it be enough to forward-declare these structures ISO 
#include'ing the whole header?

> Where does this problem show up?
>
> Any file that include linux/usb/hcd.h should include linux/usb.h first.
> IMO it would be better to fix the source files that don't do the
> includes properly.

    Yeah, let's fix the consequency instead of the cause. :-)

> Of course, people have varying opinions on this issue.  As far as I
> know, there is no fixed policy in the kernel about nested includes.

    So far, I've only encountered the dubious policy of satisfying header's 
dependencies in the files that include them is the USB tree.

> Alan Stern

WBR, Sergei

RE: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[David Laight]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 951 bytes --]

> > These warning are non-default GCC warnings. These can be seen either
> > by adding W=1 while running make (i.e. make W=1) or adding
> > -Wmissing-prototypes in KBUILD_CFLAGS in the toplevel Makefile.
> 
> By default, we don't care about 'W=1' warnings, as no one sees them, and
> they don't matter.

-Wmissing-prototypes really ought to be defined.

Some of the other warning are a little more pedantic, the most annoying
is -Wsign-compare.  OTOH a lot of code out there is clean enough to build
without any warnings and with a moderate amount of pedantry from the compiler.

OTOH just including extra headers isn't ideal - it can considerably
slow down the compilation time. There are many subsystems that don't
really separate their internal headers from their external ones.

	David

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Sergei Shtylyov]

Hello.

On 12/19/2013 07:38 PM, Alan Stern wrote:

>>>> Include header file include/linux/usb.h in include/linux/usb/hcd.h
>>>> because structures usb_device, usb_host_config and usb_interface have
>>>> their definitions in include/linux/usb.h.

>>>> This eliminates the following warning in include/linux/usb/hcd.h:
>>>> include/linux/usb/hcd.h:311:44: warning: ‘struct usb_device’ declared inside parameter list [enabled by default]
>>>> include/linux/usb/hcd.h:412:10: warning: ‘struct usb_host_config’ declared inside parameter list [enabled by default]
>>>> include/linux/usb/hcd.h:614:9: warning: ‘struct usb_interface’ declared inside parameter list [enabled by default]

>>      Rashika, would it be enough to forward-declare these structures ISO
>> #include'ing the whole header?

> I agree, that would fix the problem.

    It should also make Greg happier. :-)

>>> Where does this problem show up?

>>> Any file that include linux/usb/hcd.h should include linux/usb.h first.
>>> IMO it would be better to fix the source files that don't do the
>>> includes properly.

>>      Yeah, let's fix the consequency instead of the cause. :-)

> The _real_ cause is that the Linux source code is extremely
> complicated, and it is remarkably difficult to insure that all header
> files have no unsatisfied dependencies.  How do you suggest fixing
> _that_?

> For example, suppose A.c includes B.h, and B.h includes C.h, and C.h
> defines struct foo.  Then A.c can use struct foo freely without
> including C.h directly (and this sort of thing happens quite a lot in
> the kernel source).  But consider what happens when B.h is changed so
> that it no longer includes C.h.

    That's a whole different issue than what we're dealing with.

>>> Of course, people have varying opinions on this issue.  As far as I
>>> know, there is no fixed policy in the kernel about nested includes.

>>      So far, I've only encountered the dubious policy of satisfying header's
>> dependencies in the files that include them is the USB tree.

> Have you looked in any other places?

    I have over 500 patches in the different areas of the kernel, so 
apparently I have if I'm telling you this. The only place where my patch to 
fix a header file so that it would be self-contained has encountered a 
maintainer's resistance was linux-usb. And note that it wasn't merely a case 
like this, where incomplete structure declarations would be enough, it was the 
case where the full structure declarations were needed.

> For that matter, how do you know
> that the USB tree has such a policy?

    From Greg KH. Also, from the late David Brownell.

>  Is it documented anywhere?

    I don't think so.

> Alan Stern

WBR, Sergei

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Sergei Shtylyov]

Hello.

On 12/19/2013 07:48 PM, Alan Stern wrote:

>>> Of course, people have varying opinions on this issue.  As far as I
>>> know, there is no fixed policy in the kernel about nested includes.

>> True.  I personally prefer the policy of making all headers
>> self-contained, and then only including headers that define things used
>> in the source file.  That has the advantage of not including any
>> unnecessary headers if the dependencies shrink, and not requiring
>> changes to multiple source files if the dependencies grow.

>> Any particular objection to making the headers self-contained?

> I guess it depends on what you mean by "self-contained".  The only
> reasonable definition I can think of at the moment is that you don't
> get any errors or warnings when you compile the .h file by itself.

> But what use is that in practice?  Nobody ever compiles .h files by
> themselves.

    It's enough to verify that a .c file containing the given .h file would 
not cause errors *located in that .h file*. This is not really such an 
improbable situation, e.g. at the early stages of development. I did discover 
header fiel errors this way.

> For that matter, how can you tell that you are including only headers
> that define things used in the source file?

    I still think that's a whole different issue.

> Remove each #include line,
> one at a time, and see if you then get an error?  Do you do this after
> each change to the source file to make sure it remains true over time?

    That's what #include cleanup patches are for. Somebody has to do them from 
time to time when the #include's accumulate -- they tend to accumulate as 
people often forget to remove no longer needed one while removing some feature 
from the .c file.

> My point is that the C language design and compiler infrastructure make
> it virtually impossible to enforce any fixed policy.

    I don't really see how C language design can justify header files that 
once included, require each .c file to #include other headers ahead of them, 
each time such header is used. In my opinion, it's just crazy.

> Alan Stern

WBR, Sergei

Re: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 03:36:00PM +0530, Rashika Kheria wrote:
> Include appropriate header file include/linux/usb/otg.h in core/hcd.c
> because function usb_bus_start_enum() has its prototype declaration in
> include/linux/usb/otg.h.
> 
> This eliminates the following warning in core/hcd.c:
> drivers/usb/core/hcd.c:2295:5: warning: no previous prototype for ‘usb_bus_start_enum’ [-Wmissing-prototypes]
> 
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> ---
>  drivers/usb/core/hcd.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 6bffb8c..077be7c 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -44,6 +44,7 @@
>  
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> +#include <linux/usb/otg.h>

This patch fails to apply to my usb-next branch of my usb.git tree, so I
can't accept it.

Can you refresh it against that branch and resend it please?

thanks,

greg k-h

Re: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[Josh Triplett]

On Thu, Dec 19, 2013 at 08:58:02AM -0800, Greg Kroah-Hartman wrote:
> On Thu, Dec 19, 2013 at 10:11:45PM +0530, Rashika Kheria wrote:
> > On Thu, Dec 19, 2013 at 10:06 PM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Thu, Dec 19, 2013 at 03:36:00PM +0530, Rashika Kheria wrote:
> > >> Include appropriate header file include/linux/usb/otg.h in core/hcd.c
> > >> because function usb_bus_start_enum() has its prototype declaration in
> > >> include/linux/usb/otg.h.
> > >>
> > >> This eliminates the following warning in core/hcd.c:
> > >> drivers/usb/core/hcd.c:2295:5: warning: no previous prototype for ‘usb_bus_start_enum’ [-Wmissing-prototypes]
> > >
> > > What is generating this warning?  I don't see it here on my machines,
> > > nor do I see any of the other warnings you have fixed in this series.
> > > How do I duplicate them?
> > >
> > > thanks,
> > >
> > > greg k-h
> > 
> > Hi Greg,
> > 
> > These warning are non-default GCC warnings. These can be seen either
> > by adding W=1 while running make (i.e. make W=1) or adding
> > -Wmissing-prototypes in KBUILD_CFLAGS in the toplevel Makefile.
> 
> By default, we don't care about 'W=1' warnings, as no one sees them, and
> they don't matter.
> 
> Now if you were to see these issues with 'sparse' or just a "normal"
> build, then that might be worth fixing up.

Code that generates warnings with gcc -Wmissing-prototypes will also
generate warnings with Sparse's (on by default) -Wdecl.  (-Wdecl also
warns about data, which gcc doesn't.)

- Josh Triplett

Re: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[Josh Triplett]

On Thu, Dec 19, 2013 at 05:33:09PM -0000, David Laight wrote:
> > > These warning are non-default GCC warnings. These can be seen either
> > > by adding W=1 while running make (i.e. make W=1) or adding
> > > -Wmissing-prototypes in KBUILD_CFLAGS in the toplevel Makefile.
> > 
> > By default, we don't care about 'W=1' warnings, as no one sees them, and
> > they don't matter.
> 
> -Wmissing-prototypes really ought to be defined.

Yeah, that's the plan; this volume of patches should make it
sufficiently low-noise to allow turning it on by default.

> Some of the other warning are a little more pedantic, the most annoying
> is -Wsign-compare.  OTOH a lot of code out there is clean enough to build
> without any warnings and with a moderate amount of pedantry from the compiler.
> 
> OTOH just including extra headers isn't ideal - it can considerably
> slow down the compilation time. There are many subsystems that don't
> really separate their internal headers from their external ones.

There's a benefit to doing so, though: it ensures that the prototypes in
the header stay in sync with the definition.

- Josh Triplett

Re: [PATCH 3/7] drivers: usb: Include appropriate header file in hcd.h

[Alan Stern]

On Thu, 19 Dec 2013, Sergei Shtylyov wrote:

>     I don't really see how C language design can justify header files that 
> once included, require each .c file to #include other headers ahead of them, 
> each time such header is used. In my opinion, it's just crazy.

Okay, you've convinced me.

In this case, anyway, it makes sense to add the structure declarations
to the header file.  That's a lot less objectionable than adding a new 
include line (even though the compiler doesn't care).

By the way, if anyone cares, this discussion reminded me something
interesting.  It's sort of the opposite side of the coin, a case where
a source file would do something _different_ each time it was included.

The program itself was a fairly simple thing to calculate and print
prime numbers.  The interesting part was that this didn't happen when 
you would _run_ the program -- it happened when you _compiled_ the 
program!  Lots of preprocessor stuff to make it work.  And the only way 
to persuade the compiler to go into a loop was for the source file to 
include itself.  :-)

Alan Stern

RE: [PATCH 1/7] drivers: usb: Include appropriate header file in hcd.c

[David Laight]

> From: Josh Triplett
> On Thu, Dec 19, 2013 at 05:33:09PM -0000, David Laight wrote:
> > OTOH just including extra headers isn't ideal - it can considerably
> > slow down the compilation time. There are many subsystems that don't
> > really separate their internal headers from their external ones.
> 
> There's a benefit to doing so, though: it ensures that the prototypes in
> the header stay in sync with the definition.

I think you misunderstood what I was saying.
The header with the function prototypes has to be included when the
driver itself is built - there is a gcc warning for that as well.

The 'problem' is that if I only have:
    void foo(struct foo *);
then the C language (uselessly) scopes the declaration of 'struct foo'
to the inside of the functions - making it almost impossible to call.

All you need is an outer declaration:
    struct foo;
    void foo(struct foo *);
what you don't need is the actual definition of 'struct foo'.
Source files that need to look at the members of the structure
should be directly including the header that defines the structure.

Adding nested includes just makes it more likely that code will fail
to directly include the headers that define the structures it uses.

	David