Re: [PATCH 12/14] spidernet: check if firmware was loaded correctly

Re: [PATCH 12/14] spidernet: check if firmware was loaded correctly

[Paul Mackerras]

Arnd Bergmann writes:

> Uploading the device firmware may fail if wrong input data
> was provided by the user. This checks for the condition.
> 
> From: Jens.Osterkamp@de.ibm.com
> Cc: netdev@vger.kernel.org

This one should be sent to Jeff Garzik, along with patches 11, 13 and
14.

Paul.

[PATCH 11/14] spidernet: fix Kconfig after BPA->CELL rename

[Arnd Bergmann]

[-- Attachment #1: spidernet-with-pci-and-cell.diff --]
[-- Type: text/plain, Size: 710 bytes --]

We changed the name of the Kconfig symbols along with
the move to arch/powerpc. This one hunk got lost during
the conversion.

From: Jens.Osterkamp@de.ibm.com
Cc: netdev@vger.kernel.org
Signed-off-by: Arnd Bergmann <arndb@de.ibm.com>

Index: linux-2.6.15-rc1/drivers/net/Kconfig
===================================================================
--- linux-2.6.15-rc1.orig/drivers/net/Kconfig
+++ linux-2.6.15-rc1/drivers/net/Kconfig
@@ -2120,7 +2120,7 @@ config BNX2
 
 config SPIDER_NET
 	tristate "Spider Gigabit Ethernet driver"
-	depends on PCI && PPC_BPA
+	depends on PCI && PPC_CELL
 	help
 	  This driver supports the Gigabit Ethernet chips present on the
 	  Cell Processor-Based Blades from IBM.

--

[PATCH 12/14] spidernet: check if firmware was loaded correctly

[Arnd Bergmann]

[-- Attachment #1: spidernet-programcheck.diff --]
[-- Type: text/plain, Size: 1798 bytes --]

Uploading the device firmware may fail if wrong input data
was provided by the user. This checks for the condition.

From: Jens.Osterkamp@de.ibm.com
Cc: netdev@vger.kernel.org
Signed-off-by: Arnd Bergmann <arndb@de.ibm.com>

Index: linux-2.6.15-rc/drivers/net/spider_net.c
===================================================================
--- linux-2.6.15-rc.orig/drivers/net/spider_net.c
+++ linux-2.6.15-rc/drivers/net/spider_net.c
@@ -1836,7 +1836,7 @@ spider_net_setup_phy(struct spider_net_c
  * spider_net_download_firmware loads the firmware opened by
  * spider_net_init_firmware into the adapter.
  */
-static void
+static int 
 spider_net_download_firmware(struct spider_net_card *card,
 			     const struct firmware *firmware)
 {
@@ -1857,8 +1857,13 @@ spider_net_download_firmware(struct spid
 		}
 	}
 
+	if (spider_net_read_reg(card, SPIDER_NET_GSINIT))
+		return -EIO;
+	
 	spider_net_write_reg(card, SPIDER_NET_GSINIT,
 			     SPIDER_NET_RUN_SEQ_VALUE);
+
+	return 0;
 }
 
 /**
@@ -1909,9 +1914,8 @@ spider_net_init_firmware(struct spider_n
 		goto out;
 	}
 
-	spider_net_download_firmware(card, firmware);
-
-	err = 0;
+	if (!spider_net_download_firmware(card, firmware))
+		err = 0;
 out:
 	release_firmware(firmware);
 
Index: linux-2.6.15-rc/drivers/net/spider_net.h
===================================================================
--- linux-2.6.15-rc.orig/drivers/net/spider_net.h
+++ linux-2.6.15-rc/drivers/net/spider_net.h
@@ -155,7 +155,7 @@ extern char spider_net_driver_name[];
 /* set this first, then the FRAMENUM_VALUE */
 #define SPIDER_NET_GFXFRAMES_VALUE	0x00000000
 
-#define SPIDER_NET_STOP_SEQ_VALUE	0x00000000
+#define SPIDER_NET_STOP_SEQ_VALUE	0x007e0000
 #define SPIDER_NET_RUN_SEQ_VALUE	0x0000007e
 
 #define SPIDER_NET_PHY_CTRL_VALUE	0x00040040

--

[PATCH 13/14] spidernet: read firmware from the OF device tree

[Arnd Bergmann]

[-- Attachment #1: spidernet-fw-from-dt-2.diff --]
[-- Type: text/plain, Size: 1584 bytes --]

request_firmware() is sometimes problematic, especially
in initramfs, reading the firmware from Open Firmware
is much preferrable.

We still try to get the firmware from the file system
first, in order to support old SLOF releases and to allow
updates of the spidernet firmware without reflashing
the system.

From: Jens.Osterkamp@de.ibm.com
Cc: netdev@vger.kernel.org
Signed-off-by: Arnd Bergmann <arndb@de.ibm.com>

Index: linux-2.6.15-rc/drivers/net/spider_net.c
===================================================================
--- linux-2.6.15-rc.orig/drivers/net/spider_net.c
+++ linux-2.6.15-rc/drivers/net/spider_net.c
@@ -1895,16 +1895,27 @@ spider_net_download_firmware(struct spid
 static int
 spider_net_init_firmware(struct spider_net_card *card)
 {
-	const struct firmware *firmware;
+	struct firmware *firmware;
+	struct device_node *dn;
+	u8 *fw_prop;
 	int err = -EIO;
 
-	if (request_firmware(&firmware,
+	if (request_firmware((const struct firmware **)&firmware,
 			     SPIDER_NET_FIRMWARE_NAME, &card->pdev->dev) < 0) {
 		if (netif_msg_probe(card))
 			pr_err("Couldn't read in sequencer data file %s.\n",
 			       SPIDER_NET_FIRMWARE_NAME);
-		firmware = NULL;
-		goto out;
+
+		dn = pci_device_to_OF_node(card->pdev);
+		if (!dn)
+			goto out;
+
+		fw_prop = (u8 *)get_property(dn, "firmware", NULL);
+		if (!fw_prop)
+			goto out;
+
+		memcpy(firmware->data, fw_prop, 6 * SPIDER_NET_FIRMWARE_LEN * sizeof(u32));
+		firmware->size = 6 * SPIDER_NET_FIRMWARE_LEN * sizeof(u32);
 	}
 
 	if (firmware->size != 6 * SPIDER_NET_FIRMWARE_LEN * sizeof(u32)) {

--

[PATCH 14/14] spidernet: fix HW structures for 64 bit dma_addr_t

[Arnd Bergmann]

[-- Attachment #1: spidernet-dma_addr_t-fix-2.diff --]
[-- Type: text/plain, Size: 2460 bytes --]

The driver incorrectly used dma_addr_t to describe
HW structures and consequently broke when that type
was changed in 2.6.15-rc.

This changed spidernet to use u32 for 32 bit HW defined
structure elements.

From: Jens.Osterkamp@de.ibm.com
Cc: netdev@vger.kernel.org
Signed-off-by: Arnd Bergmann <arndb@de.ibm.com>

Index: linux-2.6.15-rc/drivers/net/spider_net.h
===================================================================
--- linux-2.6.15-rc.orig/drivers/net/spider_net.h
+++ linux-2.6.15-rc/drivers/net/spider_net.h
@@ -373,9 +373,9 @@ enum spider_net_descr_status {
 
 struct spider_net_descr {
 	/* as defined by the hardware */
-	dma_addr_t buf_addr;
+	u32 buf_addr;
 	u32 buf_size;
-	dma_addr_t next_descr_addr;
+	u32 next_descr_addr;
 	u32 dmac_cmd_status;
 	u32 result_size;
 	u32 valid_size;	/* all zeroes for tx */
Index: linux-2.6.15-rc/drivers/net/spider_net.c
===================================================================
--- linux-2.6.15-rc.orig/drivers/net/spider_net.c
+++ linux-2.6.15-rc/drivers/net/spider_net.c
@@ -480,6 +480,7 @@ static int
 spider_net_prepare_rx_descr(struct spider_net_card *card,
 			    struct spider_net_descr *descr)
 {
+	dma_addr_t buf;
 	int error = 0;
 	int offset;
 	int bufsize;
@@ -510,10 +511,11 @@ spider_net_prepare_rx_descr(struct spide
 	if (offset)
 		skb_reserve(descr->skb, SPIDER_NET_RXBUF_ALIGN - offset);
 	/* io-mmu-map the skb */
-	descr->buf_addr = pci_map_single(card->pdev, descr->skb->data,
+	buf = pci_map_single(card->pdev, descr->skb->data,
 					 SPIDER_NET_MAX_MTU,
 					 PCI_DMA_BIDIRECTIONAL);
-	if (descr->buf_addr == DMA_ERROR_CODE) {
+	descr->buf_addr = buf;
+	if (buf == DMA_ERROR_CODE) {
 		dev_kfree_skb_any(descr->skb);
 		if (netif_msg_rx_err(card))
 			pr_err("Could not iommu-map rx buffer\n");
@@ -914,15 +916,16 @@ spider_net_prepare_tx_descr(struct spide
 			    struct spider_net_descr *descr,
 			    struct sk_buff *skb)
 {
-	descr->buf_addr = pci_map_single(card->pdev, skb->data,
-					 skb->len, PCI_DMA_BIDIRECTIONAL);
-	if (descr->buf_addr == DMA_ERROR_CODE) {
+	dma_addr_t buf = pci_map_single(card->pdev, skb->data,
+					skb->len, PCI_DMA_BIDIRECTIONAL);
+	if (buf == DMA_ERROR_CODE) {
 		if (netif_msg_tx_err(card))
 			pr_err("could not iommu-map packet (%p, %i). "
 				  "Dropping packet\n", skb->data, skb->len);
 		return -ENOMEM;
 	}
 
+	descr->buf_addr = buf;
 	descr->buf_size = skb->len;
 	descr->skb = skb;
 	descr->data_status = 0;

--

Re: [PATCH 12/14] spidernet: check if firmware was loaded correctly

[Arnd Bergmann]

On Dinsdag 06 Dezember 2005 01:59, Paul Mackerras wrote:
> Arnd Bergmann writes:
> 
> > Uploading the device firmware may fail if wrong input data
> > was provided by the user. This checks for the condition.
> > 
> > From: Jens.Osterkamp@de.ibm.com
> > Cc: netdev@vger.kernel.org
> 
> This one should be sent to Jeff Garzik, along with patches 11, 13 and
> 14.

Ok.

Jens, is it ok for you if you send the network driver stuff to
jgarzik@pobox.com, Cc: netdev@vger.kernel.org yourself in the future?

	Arnd <><

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Arnd Bergmann]

On Dinsdag 06 Dezember 2005 12:38, Pekka Enberg wrote:
> On Dinsdag 06 Dezember 2005 01:51, Paul Mackerras wrote:
> > > Remind me again why spufs is under arch/powerpc/ rather than fs/ ?
> 
> On Tue, 2005-12-06 at 11:18 +0100, Arnd Bergmann wrote:
> > We had a discussion about this in August, after the patch
> > at http://patchwork.ozlabs.org/linuxppc64/patch?id=2140
> > 
> > Nobody had voiced any objections against the arch/powerpc location,
> > and Pekka had good reasons against fs/, so I changed it.
> 
> It had arch specific hooks which IMHO do not belong into fs/.

Since the discussion came up again in irc, I looked up the existing file
systems.

outside of fs/, we have the following file systems.

find -name \*.c | grep -v ^./fs | xargs grep struct.file_system_type.*=
./arch/ia64/kernel/perfmon.c:static struct file_system_type pfm_fs_type = {
./drivers/infiniband/core/uverbs_main.c:static struct file_system_type uverbs_event_fs = {
./drivers/isdn/capi/capifs.c:static struct file_system_type capifs_fs_type = {
./drivers/misc/ibmasm/ibmasmfs.c:static struct file_system_type ibmasmfs_type = {
./drivers/oprofile/oprofilefs.c:static struct file_system_type oprofilefs_type = {
./drivers/usb/core/inode.c:static struct file_system_type usb_fs_type = {
./drivers/usb/gadget/inode.c:static struct file_system_type gadgetfs_type = {
./ipc/mqueue.c:static struct file_system_type mqueue_fs_type = {
./kernel/cpuset.c:static struct file_system_type cpuset_fs_type = {
./kernel/futex.c:static struct file_system_type futex_fs_type = {
./mm/shmem.c:static struct file_system_type tmpfs_fs_type = {
./mm/tiny-shmem.c:static struct file_system_type tmpfs_fs_type = {
./net/socket.c:static struct file_system_type sock_fs_type = {
./net/sunrpc/rpc_pipe.c:static struct file_system_type rpc_pipe_fs_type = {
./security/inode.c:static struct file_system_type fs_type = {
./security/selinux/selinuxfs.c:static struct file_system_type sel_fs_type = {

In fs/, most code deals with actual files stored on a disk or similar,
with the exception of:

./fs/binfmt_misc.c:static struct file_system_type bm_fs_type = {
./fs/block_dev.c:static struct file_system_type bd_type = {
./fs/debugfs/inode.c:static struct file_system_type debug_fs_type = {
./fs/devfs/base.c:static struct file_system_type devfs_fs_type = {
./fs/devpts/inode.c:static struct file_system_type devpts_fs_type = {
./fs/eventpoll.c:static struct file_system_type eventpoll_fs_type = {
./fs/hugetlbfs/inode.c:static struct file_system_type hugetlbfs_fs_type = {
./fs/inotify.c:static struct file_system_type inotify_fs_type = {
./fs/openpromfs/inode.c:static struct file_system_type openprom_fs_type = {
./fs/pipe.c:static struct file_system_type pipe_fs_type = {
./fs/proc/root.c:static struct file_system_type proc_fs_type = {
./fs/relayfs/inode.c:static struct file_system_type relayfs_fs_type = {
./fs/sysfs/mount.c:static struct file_system_type sysfs_fs_type = {

I guess there is no strict rule where these file systems go to, e.g.
hugetlbs could just as well live near mm/shmem.c or any of those outside
of fs/ could be moved in there.

I don't really care where I put spufs, but I would prefer to move
the files only one more time at most.
Initially, they were in fs/spufs, and I moved them to
arch/powerpc/platforms/cell/spufs at Pekkas suggestion.

	Arnd <><

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Pekka Enberg]

Hi,

On Tue, 2005-12-06 at 19:49 +0100, Arnd Bergmann wrote:
> I guess there is no strict rule where these file systems go to, e.g.
> hugetlbs could just as well live near mm/shmem.c or any of those outside
> of fs/ could be moved in there.

hugetlbs does not contain architecture specific code so I don't see it
as a problem.

On Tue, 2005-12-06 at 19:49 +0100, Arnd Bergmann wrote:
> I don't really care where I put spufs, but I would prefer to move
> the files only one more time at most.
> Initially, they were in fs/spufs, and I moved them to
> arch/powerpc/platforms/cell/spufs at Pekkas suggestion.

I would prefer them to stay in arch/powerpc/. As far as I understand,
spufs will never have any use for platforms other than cell, so I really
don't see any point in putting it in fs/.

			Pekka

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Paul Mackerras]

Pekka Enberg writes:

> I would prefer them to stay in arch/powerpc/. As far as I understand,
> spufs will never have any use for platforms other than cell, so I really
> don't see any point in putting it in fs/.

The point is that people making changes to the filesystem interfaces
will be much more likely to notice and fix stuff that is under fs/
than code that is buried deep under arch/ somewhere.  Filesystems
should go under fs/ for the sake of long-term maintainability.  The
fact that it's only used on one architecture is irrelevant - you
simply make sure (with the appropriate Kconfig bits) that it's only
offered on that architecture.

Paul.

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Pekka Enberg]

Hi Paul,

On Wed, 2005-12-07 at 08:10 +1100, Paul Mackerras wrote:
> The point is that people making changes to the filesystem interfaces
> will be much more likely to notice and fix stuff that is under fs/
> than code that is buried deep under arch/ somewhere.  Filesystems
> should go under fs/ for the sake of long-term maintainability.  The
> fact that it's only used on one architecture is irrelevant - you
> simply make sure (with the appropriate Kconfig bits) that it's only
> offered on that architecture.

I think the fact that it is highly architecture specific is relevant. I
have no way of testing spufs changes except on cell, no? And if I am
developing on a cell, I probably will notice it in arch/ all the same.
So I don't quite buy your the maintenace argument.

But as Arnd said, there are no clear rules on what kind of filesystems
should go into fs/ so please do whatever you must.

			Pekka

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Nathan Lynch]

Paul Mackerras wrote:
> Pekka Enberg writes:
> 
> > I would prefer them to stay in arch/powerpc/. As far as I understand,
> > spufs will never have any use for platforms other than cell, so I really
> > don't see any point in putting it in fs/.
> 
> The point is that people making changes to the filesystem interfaces
> will be much more likely to notice and fix stuff that is under fs/
> than code that is buried deep under arch/ somewhere.  Filesystems
> should go under fs/ for the sake of long-term maintainability.  The
> fact that it's only used on one architecture is irrelevant - you
> simply make sure (with the appropriate Kconfig bits) that it's only
> offered on that architecture.

openpromfs seems to be a precedent here.  It makes sense only on sparc
and sparc64 but it lives in fs/.

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Paul Mackerras]

Pekka Enberg writes:

> I think the fact that it is highly architecture specific is relevant. I
> have no way of testing spufs changes except on cell, no? And if I am
> developing on a cell, I probably will notice it in arch/ all the same.
> So I don't quite buy your the maintenace argument.

Think about someone changing the VFS layer interface and fixing up all
the filesystems to accommodate that change.  That person is doing some
of your work for you, so you want to make it easy for him/her to find
your filesystem.  That's the sort of thing I was referring to as
maintenance.

As for changes on the cell-specific side, the people doing those
changes will know where to find it, so it isn't a problem having it in
fs/.

Having it in fs/ also means that it is more likely that people
familiar with VFS internals will look through your code and comment on
it.  I know that can be painful in the short term, but in the long
term it will lead to better code.

Paul.

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Arnd Bergmann]

Am Dienstag 06 Dezember 2005 23:19 schrieb Paul Mackerras:
> Having it in fs/ also means that it is more likely that people
> familiar with VFS internals will look through your code and comment on
> it.  I know that can be painful in the short term, but in the long
> term it will lead to better code.

Yes, that is an excellent point. How should we proceed to get the code
there?
Do you want to move the files around in your git tree or do you
prefer me to send a full set of patches again and kill the existing
copy? Obviously, I'd prefer the former, since it would mean less
work for me with the same result.

	Arnd <><

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Al Viro]

On Wed, Dec 07, 2005 at 09:19:28AM +1100, Paul Mackerras wrote:
> Think about someone changing the VFS layer interface and fixing up all
> the filesystems to accommodate that change.  That person is doing some
> of your work for you, so you want to make it easy for him/her to find
> your filesystem.  That's the sort of thing I was referring to as
> maintenance.

FWIW, I think it's not a serious argument.  Interface changes => grep time.
And that means grep over the tree anyway.
 
> As for changes on the cell-specific side, the people doing those
> changes will know where to find it, so it isn't a problem having it in
> fs/.
> 
> Having it in fs/ also means that it is more likely that people
> familiar with VFS internals will look through your code and comment on
> it.  I know that can be painful in the short term, but in the long
> term it will lead to better code.

That's solved by asking for review...

As far as I'm concerned, the only thing here that looks like a possible
reason to move the entire thing is highly unusual semantics of final
close and interesting use of VFS interfaces in spu_create().  I.e. it's
not that we have a filesystem there.

OTOH, if you go looking for analogs as far as unusual interaction with VFS
is concerned...  net/unix is unlikely to get moved.

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Paul Mackerras]

Al Viro writes:

> FWIW, I think it's not a serious argument.  Interface changes => grep time.
> And that means grep over the tree anyway.

OK, well, where would you prefer the spufs code to go?

> That's solved by asking for review...

Could you review the spufs code (i.e. the patches posted by Arnd
recently to linuxppc64-dev@ozlabs.org) please?

Thanks,
Paul.

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Pekka Enberg]

Hi Paul,

On 12/7/05, Paul Mackerras <paulus@samba.org> wrote:
> Could you review the spufs code (i.e. the patches posted by Arnd
> recently to linuxppc64-dev@ozlabs.org) please?

Why not post them to LKML?

                              Pekka

Re: [PATCH 12/14] spidernet: check if firmware was loaded correctly

[Jens Osterkamp]

Arnd Bergmann <arnd@arndb.de> wrote on 12/06/2005 11:23:39 AM:

> On Dinsdag 06 Dezember 2005 01:59, Paul Mackerras wrote:
> > Arnd Bergmann writes:
> >
> > > Uploading the device firmware may fail if wrong input data
> > > was provided by the user. This checks for the condition.
> > >
> > > From: Jens.Osterkamp@de.ibm.com
> > > Cc: netdev@vger.kernel.org
> >
> > This one should be sent to Jeff Garzik, along with patches 11, 13 and
> > 14.
>
> Ok.
>
> Jens, is it ok for you if you send the network driver stuff to
> jgarzik@pobox.com, Cc: netdev@vger.kernel.org yourself in the future?

Sure, I will do so for our next updates.

Jens

Re: [PATCH 02/14] spufs: fix local store page refcounting

[Al Viro]

On Wed, Dec 07, 2005 at 02:15:09PM +1100, Paul Mackerras wrote:
> Al Viro writes:
> 
> > FWIW, I think it's not a serious argument.  Interface changes => grep time.
> > And that means grep over the tree anyway.
> 
> OK, well, where would you prefer the spufs code to go?

Up to ppc folks, really - I don't see any serious objections to arch/powerpc/
variants; it could go there, it could go to fs/*.  Objections along the lines
of "it won't be found" are BS - any interface change is going to start with
grep over the entire tree anyway.
 
> > That's solved by asking for review...
> 
> Could you review the spufs code (i.e. the patches posted by Arnd
> recently to linuxppc64-dev@ozlabs.org) please?

If it's what you have in powerpc.git - see comments on IRC yesterday...