au1000_lowlevel_probe on au1000_eth.c

au1000_lowlevel_probe on au1000_eth.c

[Rodolfo Giometti]

Hello,

I notice that during sleep/wakeup au1000_lowlevel_probe() tries to
access to variables arcs_cmdline,prom_envp & Co.. This sometime does
an oops.

What I like to understand is if I simply have to skip accessing to
these variables during wake up or I have to save them during system
boot or what...?

Thanks,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

Re: au1000_lowlevel_probe on au1000_eth.c

[Ulrich Eckhardt]

On Tuesday 27 June 2006 00:14, Rodolfo Giometti wrote:
> I notice that during sleep/wakeup au1000_lowlevel_probe() tries to
> access to variables arcs_cmdline,prom_envp & Co.. This sometime does
> an oops.
>
> What I like to understand is if I simply have to skip accessing to
> these variables during wake up or I have to save them during system
> boot or what...?

What is the _right_ thing is difficult to answer, but the reasons for the oops 
are simple (at least they were on the platform I used): you have more than 
one device connected to the same system bus. In my case, it was the boot-PROM 
and (I think) the PCMCIA memory. So, in practice one could only access one of 
the two and you had to toggle between them. I'm not sure it is worth the 
hassle of toggling (and doing so in an orderly fashion!), I'd rather parse 
the PROM once and then store the results in RAM.

HTH

Uli

****************************************************
Visit our website at <http://www.domino-printing.com/>
****************************************************
This Email and any files transmitted with it are intended only for the person or entity to which it is addressed and may contain confidential and/or privileged material. Any reading, redistribution, disclosure or other use of, or taking of any action in reliance upon, this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient please contact the sender immediately and delete the material from your computer.

E-mail may be susceptible to data corruption, interception, viruses and unauthorised amendment and Domino UK Limited does not accept liability for any such corruption, interception, viruses or amendment or their consequences.
****************************************************

Re: au1000_lowlevel_probe on au1000_eth.c

[Rodolfo Giometti]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

On Tue, Jun 27, 2006 at 08:36:08AM +0200, Ulrich Eckhardt wrote:
> 
> What is the _right_ thing is difficult to answer, but the reasons for the oops 
> are simple (at least they were on the platform I used): you have more than 
> one device connected to the same system bus. In my case, it was the boot-PROM 
> and (I think) the PCMCIA memory. So, in practice one could only access one of 
> the two and you had to toggle between them. I'm not sure it is worth the 
> hassle of toggling (and doing so in an orderly fashion!), I'd rather parse 
> the PROM once and then store the results in RAM.

I see. But maybe the «best» thing to do is just to skip these settings
during wake up after a sleep since they are magniful only during boot
time...

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: au1000_lowlevel_probe on au1000_eth.c

[Rodolfo Giometti]

[-- Attachment #1.1: Type: text/plain, Size: 566 bytes --]

On Tue, Jun 27, 2006 at 12:14:41AM +0200, Rodolfo Giometti wrote:
> 
> I notice that during sleep/wakeup au1000_lowlevel_probe() tries to
> access to variables arcs_cmdline,prom_envp & Co.. This sometime does
> an oops.

Here my proposal to avoid oops during wake up.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[-- Attachment #1.2: patch-au1000_eth-pm-bugfix --]
[-- Type: text/plain, Size: 3434 bytes --]

diff --git a/drivers/net/au1000_eth.c b/drivers/net/au1000_eth.c
index 341fdc4..c49004a 100644
--- a/drivers/net/au1000_eth.c
+++ b/drivers/net/au1000_eth.c
@@ -84,7 +84,7 @@ MODULE_LICENSE("GPL");
 // prototypes
 static void hard_stop(struct net_device *);
 static void enable_rx_tx(struct net_device *dev);
-static int au1000_lowlevel_probe(struct net_device *ndev, void *ioaddr, void *macen_addr, int port_num);
+static int au1000_lowlevel_probe(struct net_device *ndev, void *ioaddr, void *macen_addr, int port_num, int skip_prom);
 static void au1000_lowlevel_remove(struct net_device *ndev);
 static int au1000_init(struct net_device *);
 static int au1000_open(struct net_device *);
@@ -1393,7 +1393,7 @@ static struct ethtool_ops au1000_ethtool
 };
 
 static int 
-au1000_lowlevel_probe(struct net_device *ndev, void *ioaddr, void *macen_addr, int port_num)
+au1000_lowlevel_probe(struct net_device *ndev, void *ioaddr, void *macen_addr, int port_num, int skip_prom)
 {
 	struct au1000_private *aup = ndev->priv;
 	db_dest_t *pDB, *pDBfree;
@@ -1419,24 +1419,25 @@ au1000_lowlevel_probe(struct net_device 
 	/* Setup some variables for quick register address access */
 	if (port_num == 0)
 	{
-		/* check env variables first */
-		if (!get_ethernet_addr(ethaddr)) { 
-			memcpy(au1000_mac_addr, ethaddr, sizeof(au1000_mac_addr));
-		} else {
-			/* Check command line */
-			argptr = prom_getcmdline();
-			if ((pmac = strstr(argptr, "ethaddr=")) == NULL) {
-				printk(KERN_INFO "%s: No mac address found\n", 
-						ndev->name);
-				/* use the hard coded mac addresses */
+		if (!skip_prom) {
+			/* check env variables first */
+			if (!get_ethernet_addr(ethaddr)) { 
+				memcpy(au1000_mac_addr, ethaddr, sizeof(au1000_mac_addr));
 			} else {
-				str2eaddr(ethaddr, pmac + strlen("ethaddr="));
-				memcpy(au1000_mac_addr, ethaddr, 
-						sizeof(au1000_mac_addr));
+				/* Check command line */
+				argptr = prom_getcmdline();
+				if ((pmac = strstr(argptr, "ethaddr=")) == NULL) {
+					printk(KERN_INFO "%s: No mac address found\n", 
+							ndev->name);
+					/* use the hard coded mac addresses */
+				} else {
+					str2eaddr(ethaddr, pmac + strlen("ethaddr="));
+					memcpy(au1000_mac_addr, ethaddr, 
+							sizeof(au1000_mac_addr));
+				}
 			}
 		}
-			aup->enable = (volatile u32 *) 
-				((unsigned long) macen_addr);
+		aup->enable = (volatile u32 *) ((unsigned long) macen_addr);
 		memcpy(ndev->dev_addr, au1000_mac_addr, sizeof(au1000_mac_addr));
 		setup_hw_rings(aup, MAC0_RX_DMA_ADDR, MAC0_TX_DMA_ADDR);
 		aup->mac_id = 0;
@@ -2196,7 +2197,7 @@ static int au1000_drv_probe(struct devic
 
 	/* Force the device name to a know state... */
 	sprintf(ndev->name, "au1xxx_eth(%d)", pdev->id);
-	ret = au1000_lowlevel_probe(ndev, base_addr, macen_addr, pdev->id);
+	ret = au1000_lowlevel_probe(ndev, base_addr, macen_addr, pdev->id, 0);
 	if (ret < 0) {
 		printk (KERN_ERR "%s: low level probe failed\n", DRV_NAME);  
 		goto out_free_netdev;
@@ -2304,7 +2305,7 @@ static int au1000_drv_resume(struct devi
 	if (!ndev)
 		return 0;
 
-	ret = au1000_lowlevel_probe(ndev, (void *) aup->mac, (void *) aup->enable, aup->mac_id);
+	ret = au1000_lowlevel_probe(ndev, (void *) aup->mac, (void *) aup->enable, aup->mac_id, ~0);
 	if (ret < 0) {
 		printk (KERN_ERR "%s: low level probe failed\n", DRV_NAME);  
 		return ret;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: au1000_lowlevel_probe on au1000_eth.c

[Sergei Shtylyov]

Hello.

Rodolfo Giometti wrote:
> On Tue, Jun 27, 2006 at 12:14:41AM +0200, Rodolfo Giometti wrote:

>>I notice that during sleep/wakeup au1000_lowlevel_probe() tries to
>>access to variables arcs_cmdline,prom_envp & Co.. This sometime does
>>an oops.

> Here my proposal to avoid oops during wake up.

    This is against your rewrite, if I don't mistake?

> Ciao,

> Rodolfo

WBR, Sergei

> ------------------------------------------------------------------------
> 
> diff --git a/drivers/net/au1000_eth.c b/drivers/net/au1000_eth.c
> index 341fdc4..c49004a 100644
> --- a/drivers/net/au1000_eth.c
> +++ b/drivers/net/au1000_eth.c
> @@ -1419,24 +1419,25 @@ au1000_lowlevel_probe(struct net_device 
>  	/* Setup some variables for quick register address access */
>  	if (port_num == 0)
>  	{
> -		/* check env variables first */
> -		if (!get_ethernet_addr(ethaddr)) { 
> -			memcpy(au1000_mac_addr, ethaddr, sizeof(au1000_mac_addr));
> -		} else {
> -			/* Check command line */
> -			argptr = prom_getcmdline();
> -			if ((pmac = strstr(argptr, "ethaddr=")) == NULL) {
> -				printk(KERN_INFO "%s: No mac address found\n", 
> -						ndev->name);
> -				/* use the hard coded mac addresses */
> +		if (!skip_prom) {
> +			/* check env variables first */
> +			if (!get_ethernet_addr(ethaddr)) { 
> +				memcpy(au1000_mac_addr, ethaddr, sizeof(au1000_mac_addr));
>  			} else {
> -				str2eaddr(ethaddr, pmac + strlen("ethaddr="));
> -				memcpy(au1000_mac_addr, ethaddr, 
> -						sizeof(au1000_mac_addr));
> +				/* Check command line */
> +				argptr = prom_getcmdline();
> +				if ((pmac = strstr(argptr, "ethaddr=")) == NULL) {
> +					printk(KERN_INFO "%s: No mac address found\n", 
> +							ndev->name);
> +					/* use the hard coded mac addresses */
> +				} else {
> +					str2eaddr(ethaddr, pmac + strlen("ethaddr="));
> +					memcpy(au1000_mac_addr, ethaddr, 
> +							sizeof(au1000_mac_addr));
> +				}
>  			}

    Hrm, wouldn't it be better to put this stuff into a separate function then?

WBR, Sergei

Re: au1000_lowlevel_probe on au1000_eth.c

[Rodolfo Giometti]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Thu, Jun 29, 2006 at 07:03:51PM +0400, Sergei Shtylyov wrote:
> 
>    This is against your rewrite, if I don't mistake?

Yes.

>    Hrm, wouldn't it be better to put this stuff into a separate function 
>    then?

Maybe, but I considered that these stuff are dignificative only during
boot time so I decided to do not consider them during wake up. Is that
wrong?

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: au1000_lowlevel_probe on au1000_eth.c

[Sergei Shtylyov]

Hello.

Rodolfo Giometti wrote:

>>   Hrm, wouldn't it be better to put this stuff into a separate function 
>>   then?

> Maybe, but I considered that these stuff are dignificative only during
> boot time so I decided to do not consider them during wake up. Is that
> wrong?

    No. And exactly because of this, the __init time stuff should better be 
separated (if possible)...

> Ciao,

> Rodolfo