Re: [PATCH] CRIS architecture update

Re: [PATCH] CRIS architecture update

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> ChangeSet 1.1726.1.144, 2004/06/01 08:52:29-07:00, akpm@osdl.org
> 
> 	[PATCH] CRIS architecture update
> 	
> 	From: "Mikael Starvik" <mikael.starvik@axis.com>
> 	
> 	- Lots of fixes from 2.4.
> 	
> 	- Updated for 2.6.6.
> 	
> 	- Added IDE driver
> 	
> 	Signed-off-by: Andrew Morton <akpm@osdl.org>
> 	Signed-off-by: Linus Torvalds <torvalds@osdl.org>

Who reviewed the ethernet driver?
Who reviewed the IDE driver?
Has Bart seen this new driver?
Why was this committed without first being run by the subsystem maintainers?

In ethernet, the MII phy probe is wrong (don't need at id 0 first), it 
should be using linux/mii.h bits rather than inventing its own, 
del_timer versus del_timer_sync is questionable, the newly-added full 
duplex handling seems to be incorrect (ref drivers/net/mii.c and drivers 
that use mii_if->full_duplex), and cosmetic issues I won't bother to 
mention.

In IDE, it uses virt_to_page() when building the scatter/gather list -- 
something that IMO should not have been allowed in -mm much less 
mainline -- and other yuckiness.  In the same function, it's 
questionable whether or not it breaks with lba48.  etrax_dma_intr 
doesn't appear to check for some DMA engine conditions that code 
comments elsewhere in the driver indicate _do_ occur.  The 
ATAPI-specific e100_start_dma code doesn't look like it will work with 
all current ATAPI drivers (ide-{cdrom,floppy,tape,scsi,...}.c).

I'm happy that the CRIS people resurfaced, too, but that's no reason to 
just shove an unreviewed patch into mainline.

	Jeff

RE: [PATCH] CRIS architecture update

[Mikael Starvik]

Hi,

>Who reviewed the ethernet driver?
>Who reviewed the IDE driver?
>Has Bart seen this new driver?
>Why was this committed without first being run by the subsystem
maintainers?

I am very happy if people outside Axis can find time to review patches
that only affects the CRIS architecture (note that all files are under
arch/cris and there is no way you can get these drivers unless you run 
CRIS). I have assumed that people like you and Bart have too much on 
your hands to do this. The few times I have tried to patch CRIS stuff on 
LKML the response is usally none.

>In ethernet, the MII phy probe is wrong (don't need at id 0 first), 

Why is that? Phy address == 0 is the most common value for CRIS based
hardwares.

>it should be using linux/mii.h bits rather than inventing its own, 

Yes.

>del_timer versus del_timer_sync is questionable, 

del_timer_sync is defined to del_timer for !SMP and there is no SMP
support in CRISv10.

>the newly-added full  duplex handling seems to be incorrect
>(ref drivers/net/mii.c and drivers that use mii_if->full_duplex), 

Great! I can definitely use the code in mii.c. Thanks for the pointer.

>and cosmetic issues I won't bother to mention.

Yes, I agree that there are lots of legacy non-Linux formatting. I'll fix
that up.

>In IDE, it uses virt_to_page() when building the scatter/gather list -- 
>something that IMO should not have been allowed in -mm much less 
>mainline -- and other yuckiness.  

This code is copy-pasted from ide-dma.c (ide_raw_build_sglist).
Why is it ok there and not in our driver?

>In the same function, it's questionable whether or not it breaks 
>with lba48.  

Could be. The 2.6 driver is a direct port of the 2.4 driver and
that works with lba48 but the 2.6 driver has not been verified
to work with it.

>etrax_dma_intr doesn't appear to check for some DMA engine 
>conditions that code comments elsewhere in the driver indicate
>_do_ occur.  

etrax_dma_intr checks the result of ide_dma_end but e100_dma_end
always returns 0 and there is a comment there that this could
be improved. I can add some checks of the DMA status there to 
make sure.

>The ATAPI-specific e100_start_dma code doesn't look like it will 
>work with all current ATAPI drivers (ide-{cdrom,floppy,tape,scsi,...}.c).

In e100_start_dma there are a if (!atapi) in a few places to fix 
LBA48 but there is no ATAPI specific code. The driver has only 
been tested with a disk and a CD so I agree that it may fail for 
some devices.

>I'm happy that the CRIS people resurfaced, too, but that's no reason to 
>just shove an unreviewed patch into mainline.

We are continously working on the CRIS port. Recently we have put most
of our work on support for a new CRIS sub-architecture in 2.6. 2.6 is
not really used in any CRIS-based products yet. While 2.5 was moving
fast I was reluctant to submit patches because most people don't care
if CRIS is changing things back and forth to follow the bleeding edge.

Next time I'll send the patches to the subsystem maintainers and LKML. 
I would be very happy if someone could find some time to review our
patches. 

At least the following patches are expected:
Fixes for the things you've pointed out and similar stuff
USB driver
New subarchitechture (later this year)

Thanks!
/Mikael (CRIS Maintainer)

Re: [PATCH] CRIS architecture update

[Sam Ravnborg]

On Fri, Jun 04, 2004 at 07:46:21AM +0200, Mikael Starvik wrote:
> Hi,
> 
> >Who reviewed the ethernet driver?
> >Who reviewed the IDE driver?
> >Has Bart seen this new driver?
> >Why was this committed without first being run by the subsystem
> maintainers?
> 
> I am very happy if people outside Axis can find time to review patches
> that only affects the CRIS architecture (note that all files are under
> arch/cris and there is no way you can get these drivers unless you run 
> CRIS). I have assumed that people like you and Bart have too much on 
> your hands to do this. The few times I have tried to patch CRIS stuff on 
> LKML the response is usally none.

The general rule is to locate drivers under drivers/, even the arch 
specific ones. This allows for easier grepping after users of a
given API etc.

s390 has their own directory.
arm has their own directory under driver/net/ etc.

With respect to patches, a very good approach is to follow same style
as is done for s390.
Smaller logical splitted patches, being sent out after each kernel release.
This allows LKML readers to do peer review of changes to the IDE driver,
without having to step over a lot of unrelated code.

General rule is to make easy for the reader.


	Sam

Re: [PATCH] CRIS architecture update

[Bartlomiej Zolnierkiewicz]

On Friday 04 of June 2004 07:46, Mikael Starvik wrote:
> Hi,

Hi,

> >Who reviewed the ethernet driver?
> >Who reviewed the IDE driver?
> >Has Bart seen this new driver?
> >Why was this committed without first being run by the subsystem
>
> maintainers?
>
> I am very happy if people outside Axis can find time to review patches
> that only affects the CRIS architecture (note that all files are under
> arch/cris and there is no way you can get these drivers unless you run
> CRIS). I have assumed that people like you and Bart have too much on
> your hands to do this. The few times I have tried to patch CRIS stuff on
> LKML the response is usally none.

Feel free to spam me and linux-ide@vger.kernel.org on IDE related stuff.

arch specific IDE drivers should be in drivers/ide/<arch>/

> >and cosmetic issues I won't bother to mention.
>
> Yes, I agree that there are lots of legacy non-Linux formatting. I'll fix
> that up.
>
> >In IDE, it uses virt_to_page() when building the scatter/gather list --
> >something that IMO should not have been allowed in -mm much less
> >mainline -- and other yuckiness.
>
> This code is copy-pasted from ide-dma.c (ide_raw_build_sglist).
> Why is it ok there and not in our driver?

It is OK.

> >In the same function, it's questionable whether or not it breaks
> >with lba48.
>
> Could be. The 2.6 driver is a direct port of the 2.4 driver and
> that works with lba48 but the 2.6 driver has not been verified
> to work with it.

AFAICS it should work with lba48.

> >etrax_dma_intr doesn't appear to check for some DMA engine
> >conditions that code comments elsewhere in the driver indicate
> >_do_ occur.
>
> etrax_dma_intr checks the result of ide_dma_end but e100_dma_end
> always returns 0 and there is a comment there that this could
> be improved. I can add some checks of the DMA status there to
> make sure.
>
> >The ATAPI-specific e100_start_dma code doesn't look like it will
> >work with all current ATAPI drivers (ide-{cdrom,floppy,tape,scsi,...}.c).
>
> In e100_start_dma there are a if (!atapi) in a few places to fix
> LBA48 but there is no ATAPI specific code. The driver has only
> been tested with a disk and a CD so I agree that it may fail for
> some devices.

It will probably fail on devices needing write access because
e100_dma_begin() always passes e100_read_command as
argument to e100_start_dma().

> >I'm happy that the CRIS people resurfaced, too, but that's no reason to
> >just shove an unreviewed patch into mainline.
>
> We are continously working on the CRIS port. Recently we have put most
> of our work on support for a new CRIS sub-architecture in 2.6. 2.6 is
> not really used in any CRIS-based products yet. While 2.5 was moving
> fast I was reluctant to submit patches because most people don't care
> if CRIS is changing things back and forth to follow the bleeding edge.
>
> Next time I'll send the patches to the subsystem maintainers and LKML.
> I would be very happy if someone could find some time to review our
> patches.

Good!

> At least the following patches are expected:
> Fixes for the things you've pointed out and similar stuff
> USB driver
> New subarchitechture (later this year)
>
> Thanks!
> /Mikael (CRIS Maintainer)

Thanks,
Bartlomiej

RE: [PATCH] CRIS architecture update

[Mikael Starvik]

>It will probably fail on devices needing write access because
>e100_dma_begin() always passes e100_read_command as
>argument to e100_start_dma().

e100_read_command is a variable that is either 1 for reads
or 0 for writes. This variable is set in e100_dma_write and
e100_dma_read. As long as the framework calls dma_write or 
dma_read before it calls dma_begin it will work just fine.

The driver has been tested with a ext2 filesystem on a disk 
(and the files are still there after a sync and reboot).

/Mikael

RE: [PATCH] CRIS architecture update

[Mikael Starvik]

>The general rule is to locate drivers under drivers/, even the arch 
>specific ones. This allows for easier grepping after users of a
>given API etc.

Ok, if that is a general rule I can move them (but the patch will
be large...). Generally we don't expect people to take attention 
about CRIS when making changes but it is of course nice when it 
happens.

>Smaller logical splitted patches, being sent out after each kernel release.
>This allows LKML readers to do peer review of changes to the IDE driver,
>without having to step over a lot of unrelated code.

Sure. More work for me but I'll also get more valuable feedback
and that is important.

/Mikael

Re: [PATCH] CRIS architecture update

[Sam Ravnborg]

On Mon, Jun 07, 2004 at 07:53:30AM +0200, Mikael Starvik wrote:
> >The general rule is to locate drivers under drivers/, even the arch 
> >specific ones. This allows for easier grepping after users of a
> >given API etc.
> 
> Ok, if that is a general rule I can move them (but the patch will
> be large...).

Please do so. As Bartlomiej note the rule is to keep drivers under:
drivers/<subsystem>/<arch>

The most efficient way to do the move is to send Linus a direct
mail with a simple shell script that does the moving of the files.
Along the lines of:

mkdir -p drivers/ide/cris
mv arch/cris/drivers/ide/* drivers/ide/cris

etc.

	Sam

Re: [PATCH] CRIS architecture update

[Bartlomiej Zolnierkiewicz]

On Monday 07 of June 2004 20:43, Sam Ravnborg wrote:
> On Mon, Jun 07, 2004 at 07:53:30AM +0200, Mikael Starvik wrote:
> > >The general rule is to locate drivers under drivers/, even the arch
> > >specific ones. This allows for easier grepping after users of a
> > >given API etc.
> >
> > Ok, if that is a general rule I can move them (but the patch will
> > be large...).
>
> Please do so. As Bartlomiej note the rule is to keep drivers under:
> drivers/<subsystem>/<arch>
>
> The most efficient way to do the move is to send Linus a direct
> mail with a simple shell script that does the moving of the files.
> Along the lines of:
>
> mkdir -p drivers/ide/cris
> mv arch/cris/drivers/ide/* drivers/ide/cris

If you decide to do this please rename you driver
to i.e. ide_etrax100.c as we already have ide.c file. ;-)

[ BTW thanks for explaining e100_read_command usage,
  I somehow overlooked that it is a variable not an enum ]

> etc.
>
> 	Sam