On 26 May 2012 12:38, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Sat, May 26, 2012 at 11:44:15AM +0100, Mark Einon wrote:
>> On 25 May 2012 19:32, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>> > On Fri, May 25, 2012 at 06:56:40PM +0100, Adnan Ali wrote:
>> >> This commit fixes coding style issues including braces
>> >> position and line wrapping.
>> >>
>> >> Signed-off-by: Adnan Ali <adnan.ali@xxxxxxxxxxxxxxx>
>> >> Reviewed-by: Jannis Pohlmann <jannis.pohlmann@xxxxxxxxxxxxxxx>
>> >> ---
>> >> drivers/staging/et131x/et131x.c | 11 ++++-------
>> >> 1 files changed, 4 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
>> >> index 5b11c5e..cf02336 100644
>> >> --- a/drivers/staging/et131x/et131x.c
>> >> +++ b/drivers/staging/et131x/et131x.c
>> >> @@ -85,8 +85,7 @@
>> >> MODULE_AUTHOR("Victor Soriano <vjsoriano@xxxxxxxxx>");
>> >> MODULE_AUTHOR("Mark Einon <mark.einon@xxxxxxxxx>");
>> >> MODULE_LICENSE("Dual BSD/GPL");
>> >> -MODULE_DESCRIPTION("10/100/1000 Base-T Ethernet Driver "
>> >> - "for the ET1310 by Agere Systems");
>> >> +MODULE_DESCRIPTION("10/100/1000 Base-T Ethernet Driver for the ET1310 by Agere Systems");
>> >>
>> >> /* EEPROM defines */
>> >> #define MAX_NUM_REGISTER_POLLS 1000
>> >> @@ -2967,11 +2966,10 @@ static struct rfd *nic_rx_pkts(struct et131x_adapter *adapter)
>> >> (ring_index == 0 &&
>> >> buff_index > rx_local->fbr[1]->num_entries - 1) ||
>> >> (ring_index == 1 &&
>> >> - buff_index > rx_local->fbr[0]->num_entries - 1))
>> >> + buff_index > rx_local->fbr[0]->num_entries - 1)) {
>> >> #else
>> >> - if (ring_index != 1 || buff_index > rx_local->fbr[0]->num_entries - 1)
>> >> + if (ring_index != 1 || buff_index > rx_local->fbr[0]->num_entries - 1) {
>> >> #endif
>> >> - {
>> >
>> > Mark, why do we have these nasty ifdefs? It seems like this should
>> > be an option at module load so that distros can support either way.
>> > (But that is a stock response, I haven't looked at the code).
>>
>> Hi Dan,
>>
>> That particular define used for the ifdefs has always been defined
>> since the earliest version of the driver I've encountered, so it can
>> probably be removed at some point - It's on my TODO list, and I should
>> update the in tree version at some point.
>>
>
> Yeah. Normally we would have removed it, but I saw that you fixed
> it so that it worked with it undefined so I thought maybe you wanted
> to keep it.
It's one of the things that I'd like to test once the driver is
functional again with the latest kernel.
>
>> Unfortunately due to recent changes, the driver has a few more issues
>> that just coding style to fix at present. Startup is very
>> hit-and-miss, for instance, and I've not had any external reports of
>> these bugs - which leads me to suspect that no one else is testing the
>> code on a real device... I'm slowly working my way through these.
>>
>
> You mean changes in the kernel.org version? We haven't been
> applying very many patches against this driver.
No, PCI among other things. My recent efforts have been to fix an
issue where a second PCI hotplug insert of the device causes
everything (Linux/desktop/mouse - the entire PC) to freeze
indefinitely until the device is extracted, at which point everything
carries on as normal. Subsequent hotplugs have the same behaviour.
Afterwards, no regs can be read from the device.
I've done a few bisects on the problem, and it appears sometime after
the 3.2 release.
Debugging is confounded by at least one bug that was introduced in
this time, which was fixed by the commit c9651e70ad0aa4998 (ASPM: Fix
pcie devices with non-pcie children), and by the way some bisects have
ended up nowhere I suspect there are others. Work is ongoing....
Cheers,
Mark
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
