Mailing List Archive

On Monday 10 April 2006 08:44, Denis Vlasenko wrote:
> I also spotted two bugs in the process, patches
> for those will follow.

ahd_delay(usec) is buggy. Just think how would it work
with usec == 1024*100 + 1...

Signed-off-by: Denis Vlasenko <vda@ilport.com.ua>
--
vda

[.Full quote and readded CC adresses. My fault, pressed wrong button]

Denis Vlasenko wrote:
> On Monday 10 April 2006 10:03, Rolf Eike Beer wrote:
> > Am Montag, 10. April 2006 07:49 schrieben Sie:
> > > On Monday 10 April 2006 08:44, Denis Vlasenko wrote:
> > > > I also spotted two bugs in the process, patches
> > > > for those will follow.
> > >
> > > ahd_delay(usec) is buggy. Just think how would it work
> > > with usec == 1024*100 + 1...
> >
> > This is unneeded. The biggest argument this function is ever called with
> > is 1000.
>
> I know.
>
> > I would suggest to delete this function completely. If one ever has to
> > wait for a longer period mdelay() is the right function to call.
>
> I am leaving it up to maintainer to decide. After all, the driver
> is for multiple OSes, other OS may lack mdelay().

The comment says about multiple milliseconds sleeps which just don't happen.

Eike

On Monday 10 April 2006 10:19, Rolf Eike Beer wrote:
> [.Full quote and readded CC adresses. My fault, pressed wrong button]
>
> Denis Vlasenko wrote:
> > On Monday 10 April 2006 10:03, Rolf Eike Beer wrote:
> > > Am Montag, 10. April 2006 07:49 schrieben Sie:
> > > > On Monday 10 April 2006 08:44, Denis Vlasenko wrote:
> > > > > I also spotted two bugs in the process, patches
> > > > > for those will follow.
> > > >
> > > > ahd_delay(usec) is buggy. Just think how would it work
> > > > with usec == 1024*100 + 1...
> > >
> > > This is unneeded. The biggest argument this function is ever called with
> > > is 1000.
> >
> > I know.
> >
> > > I would suggest to delete this function completely. If one ever has to
> > > wait for a longer period mdelay() is the right function to call.
> >
> > I am leaving it up to maintainer to decide. After all, the driver
> > is for multiple OSes, other OS may lack mdelay().
>
> The comment says about multiple milliseconds sleeps which just don't happen.

# grep -r ah._delay.[A-Z] .
./aic7xxx_core.c: ahc_delay(AHC_BUSRESET_DELAY);
./aic79xx_core.c: ahd_delay(AHD_BUSRESET_DELAY);
./aic79xx_core.c: ahd_delay(AHD_BUSRESET_DELAY);

I am not sure that this constant won't be redefined to something large.
If maintainer knows better, he can take care of it.
I just fixed an obvious latent bug.
--
vda
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Denis Vlasenko wrote:
...
> +++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic79xx_core.c Sun Apr 9 21:49:25 2006
...
> +#include "aic79xx_osm_o.c"
> +#include "aic79xx_inline.c"
...
> +++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic7xxx_core.c Sun Apr 9 21:49:25 2006
...
> +#include "aic7xxx_osm_o.c"
> +#include "aic7xxx_inline.c"
...

Instead of including c files with function definitions, you should add
function prototypes to header files (it seems you already did so) and
include only the header files. Include these header files in the c files
which call the functions as well as in the c files which define the
functions.

It is obviously necessary to modify the Makefile to have aic7?xx_osm_o.o
and aic7?xx_inline.o linked to an appropriate .ko file.

Furthermore, aic7?xx_inline.c are not very fitting file names since they
do not contain inline functions. aic7?xx_osm_o.c are somewhat strange
names either. Can't you move the functions into existing c files? E.g.
into those which contain most of the calls to the now de-inlined
functions. From the point of view of cross-OS driver maintenance (but
not necessarily from the point of view of Linux driver maintenance), it
may be useful to distinguish between functions used across OSs and those
used only in Linux when deciding where to move the functions.
--
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Monday 10 April 2006 11:43, Stefan Richter wrote:
> Denis Vlasenko wrote:
> ...
> > +++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic79xx_core.c Sun Apr 9 21:49:25 2006
> ...
> > +#include "aic79xx_osm_o.c"
> > +#include "aic79xx_inline.c"
> ...
> > +++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic7xxx_core.c Sun Apr 9 21:49:25 2006
> ...
> > +#include "aic7xxx_osm_o.c"
> > +#include "aic7xxx_inline.c"
> ...
>
> Instead of including c files with function definitions, you should add
> function prototypes to header files (it seems you already did so) and
> include only the header files. Include these header files in the c files
> which call the functions as well as in the c files which define the
> functions.

I will do this if maintainer will inform me that he wants it done.
Or maybe he wants it done in some other way, who knows?
I am not lazy, I am just not good at reading other peoples' minds.

> It is obviously necessary to modify the Makefile to have aic7?xx_osm_o.o
> and aic7?xx_inline.o linked to an appropriate .ko file.

I did compile test my changes.

> Furthermore, aic7?xx_inline.c are not very fitting file names since they
> do not contain inline functions. aic7?xx_osm_o.c are somewhat strange
> names either. Can't you move the functions into existing c files? E.g.

I can, but I won't without maintainer's consent.

> into those which contain most of the calls to the now de-inlined
> functions. From the point of view of cross-OS driver maintenance (but
> not necessarily from the point of view of Linux driver maintenance), it
> may be useful to distinguish between functions used across OSs and those
> used only in Linux when deciding where to move the functions.
--
vda
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Rolf Eike Beer wrote:
> Denis Vlasenko wrote:
>> I am leaving it up to maintainer to decide. After all, the driver
>> is for multiple OSes, other OS may lack mdelay().
>
> The comment says about multiple milliseconds sleeps which just don't happen.

Given what ah{c,d}_delay are (OS dependent wrappers) and how they are
used (definitely not for multi-msec delays), they should just be changed
into a #define ah{c,d}_delay(us) udelay(us) or into void inline
ah{c,d}_delay(long us) {udelay(us);}.
--
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Stefan Richter wrote:
> Rolf Eike Beer wrote:
>> Denis Vlasenko wrote:
>>> I am leaving it up to maintainer to decide. After all, the driver
>>> is for multiple OSes, other OS may lack mdelay().
>> The comment says about multiple milliseconds sleeps which just don't happen.
>
> Given what ah{c,d}_delay are (OS dependent wrappers) and how they are
> used (definitely not for multi-msec delays), they should just be changed
> into a #define ah{c,d}_delay(us) udelay(us) or into void inline
> ah{c,d}_delay(long us) {udelay(us);}.

I'd rather do a #define. Inlining simple functions is quite unneccessary
here.

Re multiplatform development: aic7{9,x}xx have ceased to be
multiplatfrom since the integration of scsi_transport_spi.
So I wouldn't worry too much about it.

Cheers,

Hannes
--
Dr. Hannes Reinecke hare@suse.de
SuSE Linux Products GmbH S390 & zSeries
Maxfeldstraße 5 +49 911 74053 688
90409 Nürnberg http://www.suse.de
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hannes Reinecke wrote:
> Re multiplatform development: aic7{9,x}xx have ceased to be
> multiplatfrom since the integration of scsi_transport_spi.
> So I wouldn't worry too much about it.

Then it may be possible to save even more than 80k of text.
--
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Denis Vlasenko wrote:
> On Monday 10 April 2006 11:43, Stefan Richter wrote:
>> It is obviously necessary to modify the Makefile to have aic7?xx_osm_o.o
>> and aic7?xx_inline.o linked to an appropriate .ko file.
>
> I did compile test my changes.

Sure, if you #include .c files (which is awkward), there are no
additional .o files which would need to be added to the Makefile.
--
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I stopped being the maintainer of these driver (for Linux at least)
almost 2 years ago. So don't bother waiting for my approval to
make changes here.

--
Justin

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Mon, 2006-04-10 at 09:19 +0200, Rolf Eike Beer wrote:
> [.Full quote and readded CC adresses. My fault, pressed wrong button]
>
> Denis Vlasenko wrote:
> > On Monday 10 April 2006 10:03, Rolf Eike Beer wrote:
> > > Am Montag, 10. April 2006 07:49 schrieben Sie:
> > > > On Monday 10 April 2006 08:44, Denis Vlasenko wrote:
> > > > > I also spotted two bugs in the process, patches
> > > > > for those will follow.
> > > >
> > > > ahd_delay(usec) is buggy. Just think how would it work
> > > > with usec == 1024*100 + 1...
> > >
> > > This is unneeded. The biggest argument this function is ever called with
> > > is 1000.
> >
> > I know.
> >
> > > I would suggest to delete this function completely. If one ever has to
> > > wait for a longer period mdelay() is the right function to call.
> >
> > I am leaving it up to maintainer to decide. After all, the driver
> > is for multiple OSes, other OS may lack mdelay().


actually this driver isn't shared anymore with other oses so it's ok to
remove (buggy) abstractions

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/