* Re: [PATCH net-next] inet: add rfc 3168 extract in front of INET_ECN_encapsulate()
From: David Miller @ 2011-10-22 5:26 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1319260268.6180.12.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 22 Oct 2011 07:11:08 +0200
> INET_ECN_encapsulate() is better understood if we can read the official
> statement.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [Qemu-devel] gcc auto-omit-frame-pointer vs msvc longjmp
From: xunxun @ 2011-10-22 5:21 UTC (permalink / raw)
To: jojelino; +Cc: gcc, Kai Tietz, qemu-devel
In-Reply-To: <4EA250E0.3080808@gmail.com>
于 2011/10/22 13:13, xunxun 写道:
> Hi, all
>
> It seems that gcc's auto-omit-frame-pointer has other problems.
>
> The example is from mingw bug tracker:
> http://sourceforge.net/tracker/?func=detail&aid=3426555&group_id=2435&atid=102435
>
> g++ -O3 main.cpp running will crash.
> g++ -O2 main.cpp running no crash.
> g++ -O3 -fno-omit-frame-pointer running no crash.
>
> I don't know in the end which optimize option defaultly contains
> this switch "-fomit-frame-pointer" on i686-pc-mingw32 or
> x86_64-w64-mingw32?
>
It crashes on Win7.
--
Best Regards,
xunxun
^ permalink raw reply
* RE: [PATCH 3/5][v2] fsl-rio: Add two ports and rapidio message units support
From: Liu Gang-B34182 @ 2011-10-22 5:15 UTC (permalink / raw)
To: Bounine, Alexandre, Kumar Gala, linuxppc-dev@ozlabs.org,
Zang Roy-R61911
Cc: Andrew Morton, linux-kernel@vger.kernel.org
In-Reply-To: <0CE8B6BE3C4AD74AB97D9D29BD24E55202367762@CORPEXCH1.na.ads.idt.com>
Hi, Alex,
Thanks for your comments, please find my replies inlines.
-----Original Message-----
From: Bounine, Alexandre [mailto:Alexandre.Bounine@idt.com]
Sent: Thursday, October 20, 2011 3:54 AM
To: Kumar Gala; linuxppc-dev@ozlabs.org
Cc: Andrew Morton; Liu Gang-B34182; linux-kernel@vger.kernel.org
Subject: RE: [PATCH 3/5][v2] fsl-rio: Add two ports and rapidio message units support
On Thu, Oct 13, 2011 at 10:09 AM, Kumar Gala wrote:
>
> From: Liu Gang <Gang.Liu@freescale.com>
>
> Usually, freescale rapidio endpoint can support one 1X or two 4X LP-
> Serial link interfaces, and rapidio message transactions can be
> implemented
by
> two
Is the number of 1x ports described correctly?
Can we have two 1x ports as well?
[Liu Gang-B34182] Yes you are right. endpoint can also have two 1x ports. I'll correct it.
> message units. This patch adds the support of two rapidio ports and
> initializes message unit 0 and message unit 1. And these ports and
> message
... skip ...
> +
> + /* Probe the master port phy type */
> + ccsr = in_be32(priv->regs_win + RIO_CCSR + i*0x20);
> + port->phy_type = (ccsr & 1) ? RIO_PHY_SERIAL :
> RIO_PHY_PARALLEL;
> + dev_info(&dev->dev, "RapidIO PHY type: %s\n",
> + (port->phy_type == RIO_PHY_PARALLEL) ?
> + "parallel" :
> + ((port->phy_type == RIO_PHY_SERIAL) ?
"serial"
> :
> + "unknown"));
> + /* Checking the port training status */
> + if (in_be32((priv->regs_win + RIO_ESCSR + i*0x20)) & 1)
{
> + dev_err(&dev->dev, "Port %d is not ready. "
> + "Try to restart connection...\n", i);
> + switch (port->phy_type) {
> + case RIO_PHY_SERIAL:
> + /* Disable ports */
> + out_be32(priv->regs_win
> + + RIO_CCSR + i*0x20, 0);
> + /* Set 1x lane */
> + setbits32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x02000000);
> + /* Enable ports */
> + setbits32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x00600000);
> + break;
> + case RIO_PHY_PARALLEL:
> + /* Disable ports */
> + out_be32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x22000000);
> + /* Enable ports */
> + out_be32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x44000000);
> + break;
> + }
Probably this may be a good moment to drop the support for parallel link.
Especially after you renamed controller to "srio" in the device tree.
[Liu Gang-B34182] I'm also considering if we should drop the parallel link support and doorbell outbound ATMU configuration.
I found some older powerpc chips support parallel link, like mpc8540 and so on. But DTS files of these chips do not support
Rapidio nodes. For example we can't find rapidio node in arch/powerpc/boot/dts/mpc8540ads.dts file. So can we conclude that
these chips with parallel rapidio link do not need the support for rapidio module and the code for parallel link can be removed?
> + msleep(100);
> + if (in_be32((priv->regs_win
> + + RIO_ESCSR + i*0x20)) & 1) {
> + dev_err(&dev->dev,
> + "Port %d restart failed.\n", i);
> + release_resource(&port->iores);
> + kfree(priv);
> + kfree(port);
> + continue;
> + }
> + dev_info(&dev->dev, "Port %d restart
success!\n", i);
> + }
> + fsl_rio_info(&dev->dev, ccsr);
> +
... skip ...
>
> struct rio_msg_regs {
> - u32 omr; /* 0xD_3000 - Outbound message 0 mode register
*/
> - u32 osr; /* 0xD_3004 - Outbound message 0 status register
*/
> + u32 omr;
> + u32 osr;
> u32 pad1;
> - u32 odqdpar; /* 0xD_300C - Outbound message 0 descriptor
> queue
> - dequeue pointer address register */
> + u32 odqdpar;
> u32 pad2;
> - u32 osar; /* 0xD_3014 - Outbound message 0 source address
> - register */
> - u32 odpr; /* 0xD_3018 - Outbound message 0 destination
port
> - register */
> - u32 odatr; /* 0xD_301C - Outbound message 0 destination
> attributes
> - Register*/
> - u32 odcr; /* 0xD_3020 - Outbound message 0 double-word
count
> - register */
> + u32 osar;
> + u32 odpr;
> + u32 odatr;
> + u32 odcr;
> u32 pad3;
> - u32 odqepar; /* 0xD_3028 - Outbound message 0 descriptor
> queue
> - enqueue pointer address register */
> + u32 odqepar;
> u32 pad4[13];
> - u32 imr; /* 0xD_3060 - Inbound message 0 mode register */
> - u32 isr; /* 0xD_3064 - Inbound message 0 status register
*/
> + u32 imr;
> + u32 isr;
> u32 pad5;
> - u32 ifqdpar; /* 0xD_306C - Inbound message 0 frame queue
> dequeue
> - pointer address register*/
> + u32 ifqdpar;
> u32 pad6;
> - u32 ifqepar; /* 0xD_3074 - Inbound message 0 frame queue
> enqueue
> - pointer address register */
> - u32 pad7[226];
> - u32 odmr; /* 0xD_3400 - Outbound doorbell mode register */
> - u32 odsr; /* 0xD_3404 - Outbound doorbell status register
*/
> + u32 ifqepar;
> + u32 pad7;
Do we need pad7 here?
[Liu Gang-B34182] Yeah, it's not required here. Forgot to remove it when re-wrote this struct.
> +};
> +
> +struct rio_dbell_regs {
> + u32 odmr;
> + u32 odsr;
> u32 res0[4];
> - u32 oddpr; /* 0xD_3418 - Outbound doorbell destination port
> - register */
... skip ...
>
> @@ -340,35 +327,45 @@ fsl_rio_dbell_handler(int irq, void
> *dev_instance)
> " sid %2.2x tid %2.2x info %4.4x\n",
> DBELL_SID(dmsg), DBELL_TID(dmsg),
DBELL_INF(dmsg));
>
> - list_for_each_entry(dbell, &port->dbells, node) {
> - if ((dbell->res->start <= DBELL_INF(dmsg)) &&
> - (dbell->res->end >= DBELL_INF(dmsg))) {
> - found = 1;
> - break;
> + for (i = 0; i < MAX_PORT_NUM; i++) {
> + if (fsl_dbell->mport[i]) {
> + list_for_each_entry(dbell,
> + &fsl_dbell->mport[i]->dbells,
node) {
> + if ((dbell->res->start
> + <= DBELL_INF(dmsg))
> + && (dbell->res->end
> + >= DBELL_INF(dmsg))) {
> + found = 1;
> + break;
> + }
> + }
> + if (found && dbell->dinb) {
> + dbell->dinb(fsl_dbell->mport[i],
> + dbell->dev_id,
DBELL_SID(dmsg),
> + DBELL_TID(dmsg),
> + DBELL_INF(dmsg));
> + break;
> + }
> }
> }
Do we need to check for matching DBELL_TID and mport destID here and scan only doorbell list attached to the right port? Otherwise this may call service routine associated with doorbell on a wrong port.
[Liu Gang-B34182] Do you mean to match DBELL_TID and mport DevID? Usually this is a reliable method, but for the rapidio module of powerpc, will encounter some problem. We set the port's DevID by
the register "Base Device ID CSR" defined in Rapidio Specification. The rapidio module of powerpc can support two ports but have only one the Base Device ID CSR. So these two ports will have the same
DevID. Although there are two registers "Alternate Device ID CSR" to separate deviceIDs for each port, they are specific registers of the freescale rapidio and can't be accessed by getting the extended feature
space block offset. For this doobell issue, in order to call a right service routine, perhaps we should ensure that different ports in different "res->start and res->end" configurations.
> - if (found) {
> - dbell->dinb(port, dbell->dev_id,
> - DBELL_SID(dmsg),
> - DBELL_TID(dmsg),
DBELL_INF(dmsg));
> - } else {
> +
> + if (!found) {
> pr_debug
> ("RIO: spurious doorbell,"
> " sid %2.2x tid %2.2x info %4.4x\n",
> DBELL_SID(dmsg), DBELL_TID(dmsg),
> DBELL_INF(dmsg));
> }
> - setbits32(&rmu->msg_regs->dmr, DOORBELL_DMR_DI);
> - out_be32(&rmu->msg_regs->dsr, DOORBELL_DSR_DIQI);
> + setbits32(&fsl_dbell->dbell_regs->dmr, DOORBELL_DMR_DI);
> + out_be32(&fsl_dbell->dbell_regs->dsr,
DOORBELL_DSR_DIQI);
> }
>
> out:
> return IRQ_HANDLED;
> }
>
... skip ...
> @@ -1114,50 +1104,48 @@ int fsl_rio_setup_rmu(struct rio_mport *mport,
> struct device_node *node) {
> struct rio_priv *priv;
> struct fsl_rmu *rmu;
> - struct rio_ops *ops;
> + u64 msg_start;
> + const u32 *msg_addr;
> + int mlen;
> + int aw;
>
> - if (!mport || !mport->priv || !node)
> - return -1;
> + if (!mport || !mport->priv)
> + return -EFAULT;
EINVAL may be better here?
[Liu Gang-B34182] Yes EINVAL will be better and I'll correct. Thanks!
> +
> + priv = mport->priv;
> +
> + if (!node) {
> + dev_warn(priv->dev, "Can't get %s property 'fsl,rmu'\n",
> + priv->dev->of_node->full_name);
> + return -EFAULT;
EINVAL as well?
[Liu Gang-B34182] Ditto.
> + }
Regards,
Alex.
^ permalink raw reply
* RE: [PATCH 3/5][v2] fsl-rio: Add two ports and rapidio message units support
From: Liu Gang-B34182 @ 2011-10-22 5:15 UTC (permalink / raw)
To: Bounine, Alexandre, Kumar Gala, linuxppc-dev@ozlabs.org,
Zang Roy-R61911
Cc: Andrew Morton, linux-kernel@vger.kernel.org
In-Reply-To: <0CE8B6BE3C4AD74AB97D9D29BD24E55202367762@CORPEXCH1.na.ads.idt.com>
Hi, Alex,
Thanks for your comments, please find my replies inlines.
-----Original Message-----
From: Bounine, Alexandre [mailto:Alexandre.Bounine@idt.com]=20
Sent: Thursday, October 20, 2011 3:54 AM
To: Kumar Gala; linuxppc-dev@ozlabs.org
Cc: Andrew Morton; Liu Gang-B34182; linux-kernel@vger.kernel.org
Subject: RE: [PATCH 3/5][v2] fsl-rio: Add two ports and rapidio message uni=
ts support
On Thu, Oct 13, 2011 at 10:09 AM, Kumar Gala wrote:
>=20
> From: Liu Gang <Gang.Liu@freescale.com>
>=20
> Usually, freescale rapidio endpoint can support one 1X or two 4X LP-=20
> Serial link interfaces, and rapidio message transactions can be=20
> implemented
by
> two
Is the number of 1x ports described correctly?
Can we have two 1x ports as well?=20
[Liu Gang-B34182] Yes you are right. endpoint can also have two 1x ports. I=
'll correct it.
> message units. This patch adds the support of two rapidio ports and=20
> initializes message unit 0 and message unit 1. And these ports and=20
> message
... skip ...
> +
> + /* Probe the master port phy type */
> + ccsr =3D in_be32(priv->regs_win + RIO_CCSR + i*0x20);
> + port->phy_type =3D (ccsr & 1) ? RIO_PHY_SERIAL :
> RIO_PHY_PARALLEL;
> + dev_info(&dev->dev, "RapidIO PHY type: %s\n",
> + (port->phy_type =3D=3D RIO_PHY_PARALLEL) ?
> + "parallel" :
> + ((port->phy_type =3D=3D RIO_PHY_SERIAL) ?
"serial"
> :
> + "unknown"));
> + /* Checking the port training status */
> + if (in_be32((priv->regs_win + RIO_ESCSR + i*0x20)) & 1)
{
> + dev_err(&dev->dev, "Port %d is not ready. "
> + "Try to restart connection...\n", i);
> + switch (port->phy_type) {
> + case RIO_PHY_SERIAL:
> + /* Disable ports */
> + out_be32(priv->regs_win
> + + RIO_CCSR + i*0x20, 0);
> + /* Set 1x lane */
> + setbits32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x02000000);
> + /* Enable ports */
> + setbits32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x00600000);
> + break;
> + case RIO_PHY_PARALLEL:
> + /* Disable ports */
> + out_be32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x22000000);
> + /* Enable ports */
> + out_be32(priv->regs_win
> + + RIO_CCSR + i*0x20,
0x44000000);
> + break;
> + }
Probably this may be a good moment to drop the support for parallel link.
Especially after you renamed controller to "srio" in the device tree.
[Liu Gang-B34182] I'm also considering if we should drop the parallel link =
support and doorbell outbound ATMU configuration.
I found some older powerpc chips support parallel link, like mpc8540 and so=
on. But DTS files of these chips do not support
Rapidio nodes. For example we can't find rapidio node in arch/powerpc/boot/=
dts/mpc8540ads.dts file. So can we conclude that
these chips with parallel rapidio link do not need the support for rapidio =
module and the code for parallel link can be removed?=20
> + msleep(100);
> + if (in_be32((priv->regs_win
> + + RIO_ESCSR + i*0x20)) & 1) {
> + dev_err(&dev->dev,
> + "Port %d restart failed.\n", i);
> + release_resource(&port->iores);
> + kfree(priv);
> + kfree(port);
> + continue;
> + }
> + dev_info(&dev->dev, "Port %d restart
success!\n", i);
> + }
> + fsl_rio_info(&dev->dev, ccsr);
> +
... skip ...
>=20
> struct rio_msg_regs {
> - u32 omr; /* 0xD_3000 - Outbound message 0 mode register
*/
> - u32 osr; /* 0xD_3004 - Outbound message 0 status register
*/
> + u32 omr;
> + u32 osr;
> u32 pad1;
> - u32 odqdpar; /* 0xD_300C - Outbound message 0 descriptor
> queue
> - dequeue pointer address register */
> + u32 odqdpar;
> u32 pad2;
> - u32 osar; /* 0xD_3014 - Outbound message 0 source address
> - register */
> - u32 odpr; /* 0xD_3018 - Outbound message 0 destination
port
> - register */
> - u32 odatr; /* 0xD_301C - Outbound message 0 destination
> attributes
> - Register*/
> - u32 odcr; /* 0xD_3020 - Outbound message 0 double-word
count
> - register */
> + u32 osar;
> + u32 odpr;
> + u32 odatr;
> + u32 odcr;
> u32 pad3;
> - u32 odqepar; /* 0xD_3028 - Outbound message 0 descriptor
> queue
> - enqueue pointer address register */
> + u32 odqepar;
> u32 pad4[13];
> - u32 imr; /* 0xD_3060 - Inbound message 0 mode register */
> - u32 isr; /* 0xD_3064 - Inbound message 0 status register
*/
> + u32 imr;
> + u32 isr;
> u32 pad5;
> - u32 ifqdpar; /* 0xD_306C - Inbound message 0 frame queue
> dequeue
> - pointer address register*/
> + u32 ifqdpar;
> u32 pad6;
> - u32 ifqepar; /* 0xD_3074 - Inbound message 0 frame queue
> enqueue
> - pointer address register */
> - u32 pad7[226];
> - u32 odmr; /* 0xD_3400 - Outbound doorbell mode register */
> - u32 odsr; /* 0xD_3404 - Outbound doorbell status register
*/
> + u32 ifqepar;
> + u32 pad7;
Do we need pad7 here?
[Liu Gang-B34182] Yeah, it's not required here. Forgot to remove it when re=
-wrote this struct.
> +};
> +
> +struct rio_dbell_regs {
> + u32 odmr;
> + u32 odsr;
> u32 res0[4];
> - u32 oddpr; /* 0xD_3418 - Outbound doorbell destination port
> - register */
... skip ...
>=20
> @@ -340,35 +327,45 @@ fsl_rio_dbell_handler(int irq, void
> *dev_instance)
> " sid %2.2x tid %2.2x info %4.4x\n",
> DBELL_SID(dmsg), DBELL_TID(dmsg),
DBELL_INF(dmsg));
>=20
> - list_for_each_entry(dbell, &port->dbells, node) {
> - if ((dbell->res->start <=3D DBELL_INF(dmsg)) &&
> - (dbell->res->end >=3D DBELL_INF(dmsg))) {
> - found =3D 1;
> - break;
> + for (i =3D 0; i < MAX_PORT_NUM; i++) {
> + if (fsl_dbell->mport[i]) {
> + list_for_each_entry(dbell,
> + &fsl_dbell->mport[i]->dbells,
node) {
> + if ((dbell->res->start
> + <=3D DBELL_INF(dmsg))
> + && (dbell->res->end
> + >=3D DBELL_INF(dmsg))) {
> + found =3D 1;
> + break;
> + }
> + }
> + if (found && dbell->dinb) {
> + dbell->dinb(fsl_dbell->mport[i],
> + dbell->dev_id,
DBELL_SID(dmsg),
> + DBELL_TID(dmsg),
> + DBELL_INF(dmsg));
> + break;
> + }
> }
> }
Do we need to check for matching DBELL_TID and mport destID here and scan o=
nly doorbell list attached to the right port? Otherwise this may call servi=
ce routine associated with doorbell on a wrong port.
[Liu Gang-B34182] Do you mean to match DBELL_TID and mport DevID? Usually t=
his is a reliable method, but for the rapidio module of powerpc, will encou=
nter some problem. We set the port's DevID by
the register "Base Device ID CSR" defined in Rapidio Specification. The rap=
idio module of powerpc can support two ports but have only one the Base Dev=
ice ID CSR. So these two ports will have the same
DevID. Although there are two registers "Alternate Device ID CSR" to separa=
te deviceIDs for each port, they are specific registers of the freescale ra=
pidio and can't be accessed by getting the extended feature
space block offset. For this doobell issue, in order to call a right servic=
e routine, perhaps we should ensure that different ports in different "res-=
>start and res->end" configurations.
> - if (found) {
> - dbell->dinb(port, dbell->dev_id,
> - DBELL_SID(dmsg),
> - DBELL_TID(dmsg),
DBELL_INF(dmsg));
> - } else {
> +
> + if (!found) {
> pr_debug
> ("RIO: spurious doorbell,"
> " sid %2.2x tid %2.2x info %4.4x\n",
> DBELL_SID(dmsg), DBELL_TID(dmsg),
> DBELL_INF(dmsg));
> }
> - setbits32(&rmu->msg_regs->dmr, DOORBELL_DMR_DI);
> - out_be32(&rmu->msg_regs->dsr, DOORBELL_DSR_DIQI);
> + setbits32(&fsl_dbell->dbell_regs->dmr, DOORBELL_DMR_DI);
> + out_be32(&fsl_dbell->dbell_regs->dsr,
DOORBELL_DSR_DIQI);
> }
>=20
> out:
> return IRQ_HANDLED;
> }
>=20
... skip ...
> @@ -1114,50 +1104,48 @@ int fsl_rio_setup_rmu(struct rio_mport *mport,=20
> struct device_node *node) {
> struct rio_priv *priv;
> struct fsl_rmu *rmu;
> - struct rio_ops *ops;
> + u64 msg_start;
> + const u32 *msg_addr;
> + int mlen;
> + int aw;
>=20
> - if (!mport || !mport->priv || !node)
> - return -1;
> + if (!mport || !mport->priv)
> + return -EFAULT;
EINVAL may be better here?
[Liu Gang-B34182] Yes EINVAL will be better and I'll correct. Thanks!
> +
> + priv =3D mport->priv;
> +
> + if (!node) {
> + dev_warn(priv->dev, "Can't get %s property 'fsl,rmu'\n",
> + priv->dev->of_node->full_name);
> + return -EFAULT;
EINVAL as well?
[Liu Gang-B34182] Ditto.
> + }
Regards,
Alex.=
^ permalink raw reply
* Re: [PATCH] staging: iio: light: Fix compiler warning in tsl2563.c
From: Maxin B John @ 2011-10-22 5:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dan Carpenter, Bryan Freed, linux-kernel, Greg Kroah-Hartman,
linux-iio, Arnd Bergmann, devel, Amit Kucheria
In-Reply-To: <4E60B63C.1030101@cam.ac.uk>
Hi,
On Fri, Sep 2, 2011 at 1:55 PM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 09/02/11 10:41, Maxin B. John wrote:
>>> Don't overwrite the error code. For example, the lower layers can
>>> return -EAGAIN and that's more useful than just returning -EIO every
>>> time.
>> Ahh.. Thanks a lot for explaining this.
>>
>>> Your fix works, but it's not very clean. Just add a "*id = ret;"
>>> line before the "return 0;" and that's it. (It doesn't make sense
>>> to pass a pointer to "id" and not use it).
>> Dan, yes, I agree with you. This fix is much much better than what I
>> had in my mind.
>>
>>> Yikes - I wonder why my various compilers don't throw that up.
>> I guess, in "iio-blue.git" tree, the 'id = 0' suppresses this warning.
> That'd do it. oops.
>
> Ideally keep the white space but doesn't really matter. Either send on
> to Greg directly or I'll add it to iio-blue and send on with the next fixes
> series - probably this afternoon.
>>
>> Signed-off-by: Maxin B. John <maxin.john@gmail.com>
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>
Sorry for the delay in replying to this mail. I am curious about the
status of this patch. I couldn't locate this patch at iio-blue.git as
it is not present in kernel.org now
(http://git.kernel.org/?p=linux/kernel/git/jic23/iio-blue.git;a=summary)
Please ignore this mail if you have already added this patch to your tree.
>> ---
>> diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
>> index f25243b..55012ff 100644
>> --- a/drivers/staging/iio/light/tsl2563.c
>> +++ b/drivers/staging/iio/light/tsl2563.c
>> @@ -226,7 +226,7 @@ static int tsl2563_read_id(struct tsl2563_chip *chip, u8 *id)
>> ret = i2c_smbus_read_byte_data(client, TSL2563_CMD | TSL2563_REG_ID);
>> if (ret < 0)
>> return ret;
>> -
>> + *id = ret;
>> return 0;
>> }
>>
>>
>
>
Warm Regards,
Maxin B. John
^ permalink raw reply
* Re: [PATCH] staging: iio: light: Fix compiler warning in tsl2563.c
From: Maxin B John @ 2011-10-22 5:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dan Carpenter, Bryan Freed, linux-kernel, Greg Kroah-Hartman,
linux-iio, Arnd Bergmann, devel, Amit Kucheria
In-Reply-To: <4E60B63C.1030101@cam.ac.uk>
Hi,
On Fri, Sep 2, 2011 at 1:55 PM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 09/02/11 10:41, Maxin B. John wrote:
>>> Don't overwrite the error code. =A0For example, the lower layers can
>>> return -EAGAIN and that's more useful than just returning -EIO every
>>> time.
>> Ahh.. Thanks a lot for explaining this.
>>
>>> Your fix works, but it's not very clean. =A0Just add a "*id =3D ret;"
>>> line before the "return 0;" and that's it. =A0(It doesn't make sense
>>> to pass a pointer to "id" and not use it).
>> Dan, yes, I agree with you. This fix is much much better than what I
>> had in my mind.
>>
>>> Yikes - I wonder why my various compilers don't throw that up.
>> I guess, in "iio-blue.git" tree, the 'id =3D 0' suppresses this warning.
> That'd do it. oops.
>
> Ideally keep the white space but doesn't really matter. Either send on
> to Greg directly or I'll add it to iio-blue and send on with the next fix=
es
> series - probably this afternoon.
>>
>> Signed-off-by: Maxin B. John <maxin.john@gmail.com>
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>
Sorry for the delay in replying to this mail. I am curious about the
status of this patch. I couldn't locate this patch at iio-blue.git as
it is not present in kernel.org now
(http://git.kernel.org/?p=3Dlinux/kernel/git/jic23/iio-blue.git;a=3Dsummary=
)
Please ignore this mail if you have already added this patch to your tree.
>> ---
>> diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/l=
ight/tsl2563.c
>> index f25243b..55012ff 100644
>> --- a/drivers/staging/iio/light/tsl2563.c
>> +++ b/drivers/staging/iio/light/tsl2563.c
>> @@ -226,7 +226,7 @@ static int tsl2563_read_id(struct tsl2563_chip *chip=
, u8 *id)
>> =A0 =A0 =A0 ret =3D i2c_smbus_read_byte_data(client, TSL2563_CMD | TSL25=
63_REG_ID);
>> =A0 =A0 =A0 if (ret < 0)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret;
>> -
>> + =A0 =A0 *id =3D ret;
>> =A0 =A0 =A0 return 0;
>> =A0}
>>
>>
>
>
Warm Regards,
Maxin B. John
^ permalink raw reply
* Re: [Qemu-devel] gcc auto-omit-frame-pointer vs msvc longjmp
From: xunxun @ 2011-10-22 5:13 UTC (permalink / raw)
To: jojelino; +Cc: gcc, Kai Tietz, qemu-devel
In-Reply-To: <j7sgfi$i66$1@dough.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 535 bytes --]
Hi, all
It seems that gcc's auto-omit-frame-pointer has other problems.
The example is from mingw bug tracker:
http://sourceforge.net/tracker/?func=detail&aid=3426555&group_id=2435&atid=102435
g++ -O3 main.cpp running will crash.
g++ -O2 main.cpp running no crash.
g++ -O3 -fno-omit-frame-pointer running no crash.
I don't know in the end which optimize option defaultly contains
this switch "-fomit-frame-pointer" on i686-pc-mingw32 or x86_64-w64-mingw32?
--
Best Regards,
xunxun
[-- Attachment #2: main.cpp --]
[-- Type: text/plain, Size: 750 bytes --]
#include <vector>
typedef void (*Func)(char*);
bool lie = false;
Func dummy = 0;
void moveToWindowsRecycler(const std::vector<int>& filesToDelete = std::vector<int>()) //throw FileError
{
if (!lie)
throw 1;
char errorMessage[20000];
Func fun = lie ? dummy : 0;
fun(errorMessage);
std::vector<int> fileNames;
for (std::vector<int>::const_iterator iter = filesToDelete.begin(); iter != filesToDelete.end(); ++iter)
fileNames.push_back(*iter);
}
void wgfdfsdgfsdgfsdg() //throw FileError
{
::moveToWindowsRecycler(); //throw FileError
}
int main()
{
try
{
moveToWindowsRecycler() ;//throw FileError
}
catch (...) {}
return 0;
}
^ permalink raw reply
* Re: Breakage in master since 6d4bb3833c
From: Michael Haggerty @ 2011-10-22 5:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: SZEDER Gábor, git discussion list
In-Reply-To: <7vehy68ejp.fsf@alter.siamese.dyndns.org>
On 10/21/2011 07:01 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Yes, you are right. Setting GIT=$(pwd)/bin-wrappers/git fixes the problem.
>
> So in short, this was a false alarm crying wolf, and there was no problem?
Correct, it was my fault for not running the locally-compiled version of
git the correct way to get consistent invocation of subprocesses. So
there was no problem.
...but there arguably *is* a metaproblem, namely that the obvious naive
way to invoke a locally-compiled test version of git without installing
it neither works correctly nor fails loudly. It sometimes works (if the
main process doesn't need to call any subprocesses), sometimes works
accidentally (if the subprocess that is used happens to have the
behavior expected by the test version) and sometimes fails bizarrely (as
in my case and other cases recently mentioned on the mailing list).
I can think of a few ugly hacks that could improve the situation:
1. Don't compile executables into the project root directory, but rather
into a subdirectory named something awful like
"do-not-run-commands-from-this-directory" with a big README.txt in the
directory explaining how the commands *should* be run. Even knowing
that one has to RTFM would be a help.
2. Include a special file like "GIT-DEV-SETUP" in the directory to which
the executables are compiled, but don't copy this file to $BINDIR when
git is installed. Teach git commands to check for the presence of
$(dirname $0)/GIT-DEV-SETUP, and if found, do the equivalent of "exec
GIT-DEV-SETUP "$@"" or maybe just read and use some values out of
GIT-DEV-SETUP to set up the correct environment. There could be some
environment variable like GIT_SETUP_DONE to prevent recursion and/or
allow this step to be bypassed.
3. Have git commands tell git subcommands what version they are
expecting (either via an environment variable or via a hidden
command-line parameter), and have the subcommand barf if the version
does not match its own internal version. This approach is more
intrusive but would also help defend against inconsistently-installed
versions (like having one version installed in /usr/bin and fragments of
another version installed in $HOME/bin).
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH net-next] inet: add rfc 3168 extract in front of INET_ECN_encapsulate()
From: Eric Dumazet @ 2011-10-22 5:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev
INET_ECN_encapsulate() is better understood if we can read the official
statement.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/inet_ecn.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/net/inet_ecn.h b/include/net/inet_ecn.h
index 2fa8d13..2fa1469 100644
--- a/include/net/inet_ecn.h
+++ b/include/net/inet_ecn.h
@@ -30,6 +30,14 @@ static inline int INET_ECN_is_capable(__u8 dsfield)
return dsfield & INET_ECN_ECT_0;
}
+/*
+ * RFC 3168 9.1.1
+ * The full-functionality option for ECN encapsulation is to copy the
+ * ECN codepoint of the inside header to the outside header on
+ * encapsulation if the inside header is not-ECT or ECT, and to set the
+ * ECN codepoint of the outside header to ECT(0) if the ECN codepoint of
+ * the inside header is CE.
+ */
static inline __u8 INET_ECN_encapsulate(__u8 outer, __u8 inner)
{
outer &= ~INET_ECN_MASK;
^ permalink raw reply related
* [U-Boot] A bit about board.c, board.c
From: Simon Glass @ 2011-10-22 5:11 UTC (permalink / raw)
To: u-boot
Hi,
Each architecture has its own board.c but they are mostly quite similar.
New features such as fdt, boot timing, trace, profiling, etc. must be
done separately for each arch, even if there are no
architecture-specific bits.
What would you say to adding something like lib/board.c which is a
simplified cleaned-up copy of one of the existing board.c files, and a
CONFIG_ARCH_GENERIC_BOARD option to select that in preference to the
architecture-specific one. Then perhaps people could try it out and we
could slowly move to it over time...
Regards,
Simon
^ permalink raw reply
* Re: [PATCH] dma-mapping: fix sync_single_range_* DMA debugging
From: FUJITA Tomonori @ 2011-10-22 5:08 UTC (permalink / raw)
To: clemens
Cc: arnd, akpm, fujita.tomonori, konrad.wilk, stefanr, linux-arch,
linux-kernel
In-Reply-To: <4E99F69F.1080701@ladisch.de>
On Sat, 15 Oct 2011 23:09:51 +0200
Clemens Ladisch <clemens@ladisch.de> wrote:
> Commit 5fd75a7850b5 (dma-mapping: remove unnecessary sync_single_range_*
> in dma_map_ops) unified not only the dma_map_ops but also the
> corresponding debug_dma_sync_* calls. This led to spurious WARN()ings
> like the following because the DMA debug code was no longer able to
> detect the DMA buffer base address without the separate offset parameter:
>
> WARNING: at lib/dma-debug.c:911 check_sync+0xce/0x446()
> firewire_ohci 0000:04:00.0: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x00000000cedaa400] [size=1024 bytes]
> Call Trace: ...
> [<ffffffff811326a5>] check_sync+0xce/0x446
> [<ffffffff81132ad9>] debug_dma_sync_single_for_device+0x39/0x3b
> [<ffffffffa01d6e6a>] ohci_queue_iso+0x4f3/0x77d [firewire_ohci]
> ...
>
> To fix this, unshare the sync_single_* and sync_single_range_*
> implementations so that we are able to call the correct debug_dma_sync_*
> functions.
>
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
> ---
> include/asm-generic/dma-mapping-common.h | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
^ permalink raw reply
* [PATCH 7/5] pretty: %G[?GS] placeholders
From: Junio C Hamano @ 2011-10-22 5:01 UTC (permalink / raw)
To: git
In-Reply-To: <1319071023-31919-1-git-send-email-gitster@pobox.com>
Add new placeholders related to the GPG signature on signed commits.
- %GG to show the raw verification message from GPG;
- %G? to show either "G" for Good, "B" for Bad;
- %GS to show the name of the signer.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The 6th is the one that works with a bogus commit with NUL in it I sent
out previously.
This concludes the series; I'll leave the design and implementation of
other useful placeholders to the list for now.
pretty.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/pretty.c b/pretty.c
index f45eb54..392d656 100644
--- a/pretty.c
+++ b/pretty.c
@@ -9,6 +9,7 @@
#include "notes.h"
#include "color.h"
#include "reflog-walk.h"
+#include "gpg-interface.h"
static char *user_format;
static struct cmt_fmt_map {
@@ -640,6 +641,12 @@ struct format_commit_context {
const struct pretty_print_context *pretty_ctx;
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
+ unsigned commit_signature_parsed:1;
+ struct {
+ char *gpg_output;
+ char good_bad;
+ char *signer;
+ } signature;
char *message;
size_t width, indent1, indent2;
@@ -822,6 +829,59 @@ static void rewrap_message_tail(struct strbuf *sb,
c->indent2 = new_indent2;
}
+static struct {
+ char result;
+ const char *check;
+} signature_check[] = {
+ { 'G', ": Good signature from " },
+ { 'B', ": BAD signature from " },
+};
+
+static void parse_signature_lines(struct format_commit_context *ctx)
+{
+ const char *buf = ctx->signature.gpg_output;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(signature_check); i++) {
+ const char *found = strstr(buf, signature_check[i].check);
+ const char *next;
+ if (!found)
+ continue;
+ ctx->signature.good_bad = signature_check[i].result;
+ found += strlen(signature_check[i].check);
+ next = strchrnul(found, '\n');
+ ctx->signature.signer = xmemdupz(found, next - found);
+ break;
+ }
+}
+
+static void parse_commit_signature(struct format_commit_context *ctx)
+{
+ struct strbuf payload = STRBUF_INIT;
+ struct strbuf signature = STRBUF_INIT;
+ struct strbuf gpg_output = STRBUF_INIT;
+ int status;
+
+ ctx->commit_signature_parsed = 1;
+
+ if (parse_signed_commit(ctx->commit->object.sha1,
+ &payload, &signature) <= 0)
+ goto out;
+ status = verify_signed_buffer(payload.buf, payload.len,
+ signature.buf, signature.len,
+ &gpg_output);
+ if (status && !gpg_output.len)
+ goto out;
+ ctx->signature.gpg_output = strbuf_detach(&gpg_output, NULL);
+ parse_signature_lines(ctx);
+
+ out:
+ strbuf_release(&gpg_output);
+ strbuf_release(&payload);
+ strbuf_release(&signature);
+}
+
+
static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
void *context)
{
@@ -974,6 +1034,30 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
return 0;
}
+ if (placeholder[0] == 'G') {
+ if (!c->commit_signature_parsed)
+ parse_commit_signature(c);
+ switch (placeholder[1]) {
+ case 'G':
+ if (c->signature.gpg_output)
+ strbuf_addstr(sb, c->signature.gpg_output);
+ break;
+ case '?':
+ switch (c->signature.good_bad) {
+ case 'G':
+ case 'B':
+ strbuf_addch(sb, c->signature.good_bad);
+ }
+ break;
+ case 'S':
+ if (c->signature.signer)
+ strbuf_addstr(sb, c->signature.signer);
+ break;
+ }
+ return 2;
+ }
+
+
/* For the rest we have to parse the commit header. */
if (!c->commit_header_parsed)
parse_commit_header(c);
@@ -1114,6 +1198,8 @@ void format_commit_message(const struct commit *commit,
if (context.message != commit->buffer)
free(context.message);
+ free(context.signature.gpg_output);
+ free(context.signature.signer);
}
static void pp_header(const struct pretty_print_context *pp,
--
1.7.7.555.g02edb3
^ permalink raw reply related
* Re: kernel 3.0: BUG: soft lockup: find_get_pages+0x51/0x110
From: Nai Xia @ 2011-10-22 5:07 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Mel Gorman, Hugh Dickins, Pawel Sikora, Andrew Morton, linux-mm,
jpiszcz, arekm, linux-kernel
In-Reply-To: <20111021174120.GJ608@redhat.com>
On Saturday 22 October 2011 01:41:20 Andrea Arcangeli wrote:
> On Fri, Oct 21, 2011 at 05:56:32PM +0200, Mel Gorman wrote:
> > On Thu, Oct 20, 2011 at 05:11:28PM +0800, Nai Xia wrote:
> > > On Mon, Oct 17, 2011 at 7:54 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> > > > On Thu, Oct 13, 2011 at 04:30:09PM -0700, Hugh Dickins wrote:
> > > >> mremap's down_write of mmap_sem, together with i_mmap_mutex/lock,
> > > >> and pagetable locks, were good enough before page migration (with its
> > > >> requirement that every migration entry be found) came in; and enough
> > > >> while migration always held mmap_sem. But not enough nowadays, when
> > > >> there's memory hotremove and compaction: anon_vma lock is also needed,
> > > >> to make sure a migration entry is not dodging around behind our back.
> > > >
> > > > For things like migrate and split_huge_page, the anon_vma layer must
> > > > guarantee the page is reachable by rmap walk at all times regardless
> > > > if it's at the old or new address.
> > > >
> > > > This shall be guaranteed by the copy_vma called by move_vma well
> > > > before move_page_tables/move_ptes can run.
> > > >
> > > > copy_vma obviously takes the anon_vma lock to insert the new "dst" vma
> > > > into the anon_vma chains structures (vma_link does that). That before
> > > > any pte can be moved.
> > > >
> > > > Because we keep two vmas mapped on both src and dst range, with
> > > > different vma->vm_pgoff that is valid for the page (the page doesn't
> > > > change its page->index) the page should always find _all_ its pte at
> > > > any given time.
> > > >
> > > > There may be other variables at play like the order of insertion in
> > > > the anon_vma chain matches our direction of copy and removal of the
> > > > old pte. But I think the double locking of the PT lock should make the
> > > > order in the anon_vma chain absolutely irrelevant (the rmap_walk
> > > > obviously takes the PT lock too), and furthermore likely the
> > > > anon_vma_chain insertion is favorable (the dst vma is inserted last
> > > > and checked last). But it shouldn't matter.
> > >
> > > I happened to be reading these code last week.
> > >
> > > And I do think this order matters, the reason is just quite similar why we
> > > need i_mmap_lock in move_ptes():
> > > If rmap_walk goes dst--->src, then when it first look into dst, ok, the
> >
> > You might be right in that the ordering matters. We do link new VMAs at
>
> Yes I also think ordering matters as I mentioned in the previous email
> that Nai answered to.
>
> > the end of the list in anon_vma_chain_list so remove_migrate_ptes should
> > be walking from src->dst.
>
> Correct. Like I mentioned in that previous email that Nai answered,
> that wouldn't be ok only if vma_merge succeeds and I didn't change my mind
> about that...
>
> copy_vma is only called by mremap so supposedly that path can
> trigger. Looks like I was wrong about vma_merge being able to succeed
> in copy_vma, and if it does I still think it's a problem as we have no
> ordering guarantee.
>
> The only other place that depends on the anon_vma_chain order is fork,
> and there, no vma_merge can happen, so that is safe.
>
> > If remove_migrate_pte finds src first, it will remove the pte and the
> > correct version will get copied. If move_ptes runs between when
> > remove_migrate_ptes moves from src to dst, then the PTE at dst will
> > still be correct.
>
> The problem is rmap_walk will search dst before src. So it will do
> nothing on dst. Then mremap moves the pte from src to dst. When rmap
> walk then checks "src" it finds nothing again.
>
> > > pte is not there, and it happily skip it and release the PTL.
> > > Then just before it look into src, move_ptes() comes in, takes the locks
> > > and moves the pte from src to dst. And then when rmap_walk() look
> > > into src, it will find an empty pte again. The pte is still there,
> > > but rmap_walk() missed it !
> > >
> >
> > I believe the ordering is correct though and protects us in this case.
>
> Normally it is, the only problem is vma_merge succeeding I think.
>
> > > IMO, this can really happen in case of vma_merge() succeeding.
> > > Imagine that src vma is lately faulted and in anon_vma_prepare()
> > > it got a same anon_vma with an existing vma ( named evil_vma )through
> > > find_mergeable_anon_vma(). This can potentially make the vma_merge() in
> > > copy_vma() return with evil_vma on some new relocation request. But src_vma
> > > is really linked _after_ evil_vma/new_vma/dst_vma.
> > > In this way, the ordering protocol of anon_vma chain is broken.
> > > This should be a rare case because I think in most cases
> > > if two VMAs can reusable_anon_vma() they were already merged.
> > >
> > > How do you think ?
> > >
>
> I tried to understand the above scenario yesterday but with 12 hour
> of travel on me I just couldn't.
Oh,yes, the first hypothesis was actually a vague feeling that things
might go wrong in that direction. The details in it was somewhat
missleading. But following that direction, I found the 2nd clear
hypothesis that leads to this bug step by step.
>
> Yesterday however I thought of another simpler case:
>
> part of a vma is moved with mremap elsewhere. Then it is moved back to
> its original place. So then vma_merge will succeed, and the "src" of
> mremap is now queued last in anon_vma_chain, wrong ordering.
Oh, yes, partial mremaping will do the trick. I was too addicted to find
a case when two VMAs missed a normal merge chance but will merge later
on. The only thing I can find by now is that ENOMEM is vma_adjust().
Partial mremaping is a simpler case and definitely more likey to happen.
>
> Today I read an email from Nai who showed apparently the same scenario
> I was thinking, without evil vmas or stuff.
>
> I have an hard time to imagine a vma_merge succeeding on a vma that
> isn't going back to its original place. The vm_pgoff + vma->anon_vma
> checks should keep some linarity so going back to the original place
> sounds the only way vma_merge can succeed in copy_vma. But still it
> can happen in that case I think (so not sure how the above scenario
> with an evil_vma could ever happen if it has a different anon_vma and
> it's not a part of a vma that is going back to its original place like
> in the second scenario Nai also posted about).
>
> That me and Nai had same scenario hypothesis indipendentely (second
> Nai hypoteisis not the first quoted above), plus copy_vma doing
> vma_merge and being only called by mremap, sounds like it can really
> happen.
>
> > Despite the comments in anon_vma_compatible(), I would expect that VMAs
> > that can share an anon_vma from find_mergeable_anon_vma() will also get
> > merged. When the new VMA is created, it will be linked in the usual
> > manner and the oldest->newest ordering is what is required. That's not
> > that important though.
> >
> > What is important is if mremap is moving src to a dst that is adjacent
> > to another anon_vma. If src has never been faulted, it's not an issue
> > because there are also no migration PTEs. If src has been faulted, then
> > is_mergeable_anon_vma() should fail as anon_vma1 != anon_vma2 and they
> > are not compatible. The ordering is preserved and we are still ok.
>
> I was thinking along these lines, the only pitfall should be when
> something is moved and put back into its original place. When it is
> moved, a new vma is created and queued last. When it's put back to its
> original location, vma_merge will succeed, and "src" is now the
> previous "dst" so queued last and that breaks.
>
> > All that said, while I don't think there is a problem, I can't convince
> > myself 100% of it. Andrea, can you spot a flaw?
>
> I think Nai's correct, only second hypothesis though.
>
> We have two options:
>
> 1) we remove the vma_merge call from copy_vma and we do the vma_merge
> manually after mremap succeed (so then we're as safe as fork is and we
> relay on the ordering). No locks but we'll just do 1 more allocation
> for one addition temporary vma that will be removed after mremap
> completed.
>
> 2) Hugh's original fix.
>
> First option probably is faster and prefereable, the vma_merge there
> should only trigger when putting things back to origin I suspect, and
> never with random mremaps, not sure how common it is to put things
> back to origin. If we're in a hurry we can merge Hugh's patch and
> optimize it later. We can still retain the migrate fix if we intend to
> take way number 1 later. I didn't like too much migrate doing
> speculative access on ptes that it can't miss or it'll crash anyway.
Me too, I think it's error-prone or at least we must be very careful
of its not doing sth evil. If the speculative access does not save
too much of the time, we need not brother to waste our mind power
over it.
>
> Said that the fix merged upstream is 99% certain to fix things in
> practice already so I doubt we're in hurry. And if things go wrong
> these issues don't go unnoticed and they shouldn't corrupt memory even
> if they trigger. 100% certain it can't do damage (other than a BUG_ON)
> for split_huge_page as I count the pmds encountered in the rmap_walk
> when I set the splitting bit, and I compare that count with
> page_mapcount and BUG_ON if they don't match, and later I repeat the
> same comparsion in the second rmap_walk that establishes the pte and
> downgrades the hugepmd to pmd, and BUG_ON again if they don't match
> with the previous rmap_walk count. It may be possible to trigger the
> BUG_ON with some malicious activity but it won't be too easy either
> because it's not an instant thing, still a race had to trigger and
> it's hard to reproduce.
>
> The anon_vma lock is quite a wide lock as it's shared by all parents
> anon_vma_chains too, slab allocation from local cpu may actually be
> faster in some condition (even when the slab allocation is
> superflous). But then I'm not sure. So I'm not against applying Hugh's
> fix even for the long run. I wouldn't git revert the migration change,
> but then if we go with Hugh's fix probably it'd be safe.
>
Yeah, anon_vma root lock is a big lock. And JFYI, actually I am doing
some very nasty hacking on anon_vma and one of the side effects is
breaking the root lock into pieces. But this area is pretty
convolved by many racing conditions. I hope some day I will finally make
my patch work and have your precious review of it. :-)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: kernel 3.0: BUG: soft lockup: find_get_pages+0x51/0x110
From: Nai Xia @ 2011-10-22 5:07 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Mel Gorman, Hugh Dickins, Pawel Sikora, Andrew Morton, linux-mm,
jpiszcz, arekm, linux-kernel
In-Reply-To: <20111021174120.GJ608@redhat.com>
On Saturday 22 October 2011 01:41:20 Andrea Arcangeli wrote:
> On Fri, Oct 21, 2011 at 05:56:32PM +0200, Mel Gorman wrote:
> > On Thu, Oct 20, 2011 at 05:11:28PM +0800, Nai Xia wrote:
> > > On Mon, Oct 17, 2011 at 7:54 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> > > > On Thu, Oct 13, 2011 at 04:30:09PM -0700, Hugh Dickins wrote:
> > > >> mremap's down_write of mmap_sem, together with i_mmap_mutex/lock,
> > > >> and pagetable locks, were good enough before page migration (with its
> > > >> requirement that every migration entry be found) came in; and enough
> > > >> while migration always held mmap_sem. But not enough nowadays, when
> > > >> there's memory hotremove and compaction: anon_vma lock is also needed,
> > > >> to make sure a migration entry is not dodging around behind our back.
> > > >
> > > > For things like migrate and split_huge_page, the anon_vma layer must
> > > > guarantee the page is reachable by rmap walk at all times regardless
> > > > if it's at the old or new address.
> > > >
> > > > This shall be guaranteed by the copy_vma called by move_vma well
> > > > before move_page_tables/move_ptes can run.
> > > >
> > > > copy_vma obviously takes the anon_vma lock to insert the new "dst" vma
> > > > into the anon_vma chains structures (vma_link does that). That before
> > > > any pte can be moved.
> > > >
> > > > Because we keep two vmas mapped on both src and dst range, with
> > > > different vma->vm_pgoff that is valid for the page (the page doesn't
> > > > change its page->index) the page should always find _all_ its pte at
> > > > any given time.
> > > >
> > > > There may be other variables at play like the order of insertion in
> > > > the anon_vma chain matches our direction of copy and removal of the
> > > > old pte. But I think the double locking of the PT lock should make the
> > > > order in the anon_vma chain absolutely irrelevant (the rmap_walk
> > > > obviously takes the PT lock too), and furthermore likely the
> > > > anon_vma_chain insertion is favorable (the dst vma is inserted last
> > > > and checked last). But it shouldn't matter.
> > >
> > > I happened to be reading these code last week.
> > >
> > > And I do think this order matters, the reason is just quite similar why we
> > > need i_mmap_lock in move_ptes():
> > > If rmap_walk goes dst--->src, then when it first look into dst, ok, the
> >
> > You might be right in that the ordering matters. We do link new VMAs at
>
> Yes I also think ordering matters as I mentioned in the previous email
> that Nai answered to.
>
> > the end of the list in anon_vma_chain_list so remove_migrate_ptes should
> > be walking from src->dst.
>
> Correct. Like I mentioned in that previous email that Nai answered,
> that wouldn't be ok only if vma_merge succeeds and I didn't change my mind
> about that...
>
> copy_vma is only called by mremap so supposedly that path can
> trigger. Looks like I was wrong about vma_merge being able to succeed
> in copy_vma, and if it does I still think it's a problem as we have no
> ordering guarantee.
>
> The only other place that depends on the anon_vma_chain order is fork,
> and there, no vma_merge can happen, so that is safe.
>
> > If remove_migrate_pte finds src first, it will remove the pte and the
> > correct version will get copied. If move_ptes runs between when
> > remove_migrate_ptes moves from src to dst, then the PTE at dst will
> > still be correct.
>
> The problem is rmap_walk will search dst before src. So it will do
> nothing on dst. Then mremap moves the pte from src to dst. When rmap
> walk then checks "src" it finds nothing again.
>
> > > pte is not there, and it happily skip it and release the PTL.
> > > Then just before it look into src, move_ptes() comes in, takes the locks
> > > and moves the pte from src to dst. And then when rmap_walk() look
> > > into src, it will find an empty pte again. The pte is still there,
> > > but rmap_walk() missed it !
> > >
> >
> > I believe the ordering is correct though and protects us in this case.
>
> Normally it is, the only problem is vma_merge succeeding I think.
>
> > > IMO, this can really happen in case of vma_merge() succeeding.
> > > Imagine that src vma is lately faulted and in anon_vma_prepare()
> > > it got a same anon_vma with an existing vma ( named evil_vma )through
> > > find_mergeable_anon_vma(). This can potentially make the vma_merge() in
> > > copy_vma() return with evil_vma on some new relocation request. But src_vma
> > > is really linked _after_ evil_vma/new_vma/dst_vma.
> > > In this way, the ordering protocol of anon_vma chain is broken.
> > > This should be a rare case because I think in most cases
> > > if two VMAs can reusable_anon_vma() they were already merged.
> > >
> > > How do you think ?
> > >
>
> I tried to understand the above scenario yesterday but with 12 hour
> of travel on me I just couldn't.
Oh,yes, the first hypothesis was actually a vague feeling that things
might go wrong in that direction. The details in it was somewhat
missleading. But following that direction, I found the 2nd clear
hypothesis that leads to this bug step by step.
>
> Yesterday however I thought of another simpler case:
>
> part of a vma is moved with mremap elsewhere. Then it is moved back to
> its original place. So then vma_merge will succeed, and the "src" of
> mremap is now queued last in anon_vma_chain, wrong ordering.
Oh, yes, partial mremaping will do the trick. I was too addicted to find
a case when two VMAs missed a normal merge chance but will merge later
on. The only thing I can find by now is that ENOMEM is vma_adjust().
Partial mremaping is a simpler case and definitely more likey to happen.
>
> Today I read an email from Nai who showed apparently the same scenario
> I was thinking, without evil vmas or stuff.
>
> I have an hard time to imagine a vma_merge succeeding on a vma that
> isn't going back to its original place. The vm_pgoff + vma->anon_vma
> checks should keep some linarity so going back to the original place
> sounds the only way vma_merge can succeed in copy_vma. But still it
> can happen in that case I think (so not sure how the above scenario
> with an evil_vma could ever happen if it has a different anon_vma and
> it's not a part of a vma that is going back to its original place like
> in the second scenario Nai also posted about).
>
> That me and Nai had same scenario hypothesis indipendentely (second
> Nai hypoteisis not the first quoted above), plus copy_vma doing
> vma_merge and being only called by mremap, sounds like it can really
> happen.
>
> > Despite the comments in anon_vma_compatible(), I would expect that VMAs
> > that can share an anon_vma from find_mergeable_anon_vma() will also get
> > merged. When the new VMA is created, it will be linked in the usual
> > manner and the oldest->newest ordering is what is required. That's not
> > that important though.
> >
> > What is important is if mremap is moving src to a dst that is adjacent
> > to another anon_vma. If src has never been faulted, it's not an issue
> > because there are also no migration PTEs. If src has been faulted, then
> > is_mergeable_anon_vma() should fail as anon_vma1 != anon_vma2 and they
> > are not compatible. The ordering is preserved and we are still ok.
>
> I was thinking along these lines, the only pitfall should be when
> something is moved and put back into its original place. When it is
> moved, a new vma is created and queued last. When it's put back to its
> original location, vma_merge will succeed, and "src" is now the
> previous "dst" so queued last and that breaks.
>
> > All that said, while I don't think there is a problem, I can't convince
> > myself 100% of it. Andrea, can you spot a flaw?
>
> I think Nai's correct, only second hypothesis though.
>
> We have two options:
>
> 1) we remove the vma_merge call from copy_vma and we do the vma_merge
> manually after mremap succeed (so then we're as safe as fork is and we
> relay on the ordering). No locks but we'll just do 1 more allocation
> for one addition temporary vma that will be removed after mremap
> completed.
>
> 2) Hugh's original fix.
>
> First option probably is faster and prefereable, the vma_merge there
> should only trigger when putting things back to origin I suspect, and
> never with random mremaps, not sure how common it is to put things
> back to origin. If we're in a hurry we can merge Hugh's patch and
> optimize it later. We can still retain the migrate fix if we intend to
> take way number 1 later. I didn't like too much migrate doing
> speculative access on ptes that it can't miss or it'll crash anyway.
Me too, I think it's error-prone or at least we must be very careful
of its not doing sth evil. If the speculative access does not save
too much of the time, we need not brother to waste our mind power
over it.
>
> Said that the fix merged upstream is 99% certain to fix things in
> practice already so I doubt we're in hurry. And if things go wrong
> these issues don't go unnoticed and they shouldn't corrupt memory even
> if they trigger. 100% certain it can't do damage (other than a BUG_ON)
> for split_huge_page as I count the pmds encountered in the rmap_walk
> when I set the splitting bit, and I compare that count with
> page_mapcount and BUG_ON if they don't match, and later I repeat the
> same comparsion in the second rmap_walk that establishes the pte and
> downgrades the hugepmd to pmd, and BUG_ON again if they don't match
> with the previous rmap_walk count. It may be possible to trigger the
> BUG_ON with some malicious activity but it won't be too easy either
> because it's not an instant thing, still a race had to trigger and
> it's hard to reproduce.
>
> The anon_vma lock is quite a wide lock as it's shared by all parents
> anon_vma_chains too, slab allocation from local cpu may actually be
> faster in some condition (even when the slab allocation is
> superflous). But then I'm not sure. So I'm not against applying Hugh's
> fix even for the long run. I wouldn't git revert the migration change,
> but then if we go with Hugh's fix probably it'd be safe.
>
Yeah, anon_vma root lock is a big lock. And JFYI, actually I am doing
some very nasty hacking on anon_vma and one of the side effects is
breaking the root lock into pieces. But this area is pretty
convolved by many racing conditions. I hope some day I will finally make
my patch work and have your precious review of it. :-)
^ permalink raw reply
* [U-Boot] [RFC PATCH 0/2] bootstage: record and publish boot progress timing
From: Simon Glass @ 2011-10-22 5:07 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1318790446-24792-1-git-send-email-sjg@chromium.org>
Hi,
On Sun, Oct 16, 2011 at 11:40 AM, Simon Glass <sjg@chromium.org> wrote:
> This patch is the first baby step in creating a simple boot progress timing
> feature in U-Boot.
>
> Previous discussion on this were here:
>
> http://lists.denx.de/pipermail/u-boot/2011-May/thread.html#92584
>
> A request was made to unify this with show_boot_progress(). There are many
> ways to do this, and I propose the following:
>
> 1. Create bootstage.h with the intent of replacing integers with enums. This
> will allow the values to be easily changed or rationalised later as required.
> It also makes it more explicit that (for example) 15 means we are about to
> run an OS.
>
> 2. Change show_boot_progress() to use these enums. This first patch just
> shows doing this with one (commonly used) integer value.
>
> 3. Create bootstage.c file which permits recording of bootstage timestamps.
>
> 4. Change the existing show_boot_progress() handlers within board files to
> use a provided bootstage progress handler instead. This is easy enough as
> there are few users.
>
> 5. Change show_boot_progress() to call into bootstage, which in turns calls
> the board-provided progress handler, if defined, after recording a timestamp.
> The function signature will stay the same for now.
Any comments on this please? I would like to get into this over the
next week or two.
Regards,
Simon
>
> Please advise if you don't like the above approach or have a better idea.
> Also I am not sure how best to deal with these pan-arch and pan-maintainer
> patches. Should they be split into lots of little patches for each or can
> they be applied to U-Boot's master so everyone can pick them up? It is not
> as bad as it sounds since few files make a lot of use of
> show_boot_progress().
>
> If I get as far as 5, I will think about where to go next then. Ultimately I
> would like boot timing available from before U-Boot to user space in Linux.
> See RFC patch for Linux here:
>
> http://lwn.net/Articles/460432/
>
> and discussion here:
>
> http://comments.gmane.org/gmane.linux.kernel/1194861
>
Regards,
Simon
>
>
> Simon Glass (2):
> ?Create an initial header for boot progress integers
> ?bootstage: Make use of BOOTSTAGE_ID_RUN_OS in show_boot_progress()
>
> ?arch/arm/lib/bootm.c ? ? ? ? ? ?| ? ?2 +-
> ?arch/avr32/lib/bootm.c ? ? ? ? ?| ? ?2 +-
> ?arch/m68k/lib/bootm.c ? ? ? ? ? | ? ?2 +-
> ?arch/microblaze/lib/bootm.c ? ? | ? ?2 +-
> ?arch/mips/lib/bootm.c ? ? ? ? ? | ? ?2 +-
> ?arch/mips/lib/bootm_qemu_mips.c | ? ?2 +-
> ?arch/powerpc/lib/bootm.c ? ? ? ?| ? ?2 +-
> ?board/csb226/csb226.c ? ? ? ? ? | ? 12 +++++++++---
> ?board/innokom/innokom.c ? ? ? ? | ? ?8 +++++---
> ?board/pcs440ep/pcs440ep.c ? ? ? | ? ?3 +--
> ?board/sixnet/sixnet.c ? ? ? ? ? | ? ?2 +-
> ?common/cmd_bootm.c ? ? ? ? ? ? ?| ? ?8 ++++----
> ?include/bootstage.h ? ? ? ? ? ? | ? 32 ++++++++++++++++++++++++++++++++
> ?13 files changed, 59 insertions(+), 20 deletions(-)
> ?create mode 100644 include/bootstage.h
>
> --
> 1.7.3.1
>
>
^ permalink raw reply
* [U-Boot] [PATCH 3/9] arm: Move CP15 init out of cpu_init_crit()
From: Simon Glass @ 2011-10-22 5:05 UTC (permalink / raw)
To: u-boot
In-Reply-To: <4EA1F12A.5090108@aribaud.net>
Hi Albert,
On Fri, Oct 21, 2011 at 3:24 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Le 21/10/2011 23:54, Simon Glass a ?crit :
>>
>> Hi Albert,
>>
>> On Fri, Oct 21, 2011 at 2:43 PM, Albert ARIBAUD
>> <albert.u.boot@aribaud.net> ?wrote:
>>>
>>> Le 21/10/2011 23:18, Simon Glass a ?crit :
>>>>
>>>> Hi Albert,
>>>>
>>>> On Fri, Oct 21, 2011 at 1:58 PM, Albert ARIBAUD
>>>> <albert.u.boot@aribaud.net> ? ?wrote:
>>>>>
>>>>> Le 13/10/2011 23:05, Simon Glass a ?crit :
>>>>>>
>>>>>> Some SOCs have do not start up with their 'main' CPU. The first U-Boot
>>>>>> code may then be executed with a CPU which does not have a CP15, or
>>>>>> not
>>>>>> a
>>>>>> useful one.
>>>>>>
>>>>>> Here we split the initialization of CP15 into a separate call, which
>>>>>> can
>>>>>> be performed later if required.
>>>>>>
>>>>>> Once the main CPU is running, you should call cpu_init_cp15() to
>>>>>> perform
>>>>>> this init as early as possible.
>>>>>>
>>>>>> Existing ARMv7 boards which define CONFIG_SKIP_LOWLEVEL_INIT should
>>>>>> not
>>>>>> need to change, this CP15 init is still skipped in that case. The only
>>>>>> impact for these boards is that the cpu_init_cp15() will be available
>>>>>> even if it is never used on these boards.
>>>>>
>>>>> I'm not too sure I understand how this is working: if you are moving
>>>>> cp15
>>>>> init to later, it will still be done by the same core that would have
>>>>> done
>>>>> it earlier, won't it?
>>>>>
>>>>> IOW, I would like to better understand how this boot core/main core
>>>>> business
>>>>> works. How does the main core start execution? At which address? Does
>>>>> it
>>>>> go
>>>>> through its reset vector? Do the cores share the same location for
>>>>> reset
>>>>> vectors? Etc.
>>>>
>>>> For Tegra is it like this:
>>>>
>>>> The ARM7 CPU (called AVP for Audio Video Processor) starts up first.
>>>> It runs the boot ROM and then U-Boot and gets as far as
>>>> arch_cpu_init(). The AVP does not have a CP15 or a cache so cannot run
>>>> the CP15 init code. The AVP then starts up the first Cortex-A9 (an
>>>> ARMv7 architecture CPU). This CPU (the main core, if you like) starts
>>>> from the same address as the first one (i.e. the start of U-Boot). It
>>>> is as if this is the core that we really wanted to use, but it wasn't
>>>> available initially. This main core runs through arch_cpu_init() and
>>>> sails into board_init_f(). At this point no CP15 init has been done.
>>>
>>> Thanks. So what this amounts to is, both cores will run the same binary,
>>> and
>>> I assume the AVP will shut itself off once the A9 runs. But what I don't
>>> get
>>
>> Yes
>>
>>> is, if A9 goes through the same sequence of code as AVP, then it will
>>> execute cp15 where is is not, won't it?
>>
>> On Tegra we have CONFIG_SKIP_LOWLEVEL_INIT set to avoid this. That
>> much is already supported by U-Boot.
>
> I still don't get it: if you have lowlevel init skipped for one core, it
> will be for the other as well.
OK so let's run through the U-Boot code in order for each of the two CPUs:
First AVP:
_start
skips lowlevel init (so no CP15)
board_init_f
arch_cpu_init
(at this point it starts up the A9 and halts)
A9:
_start
skips lowlevel init (so no CP15)
board_init_f
arch_cpu_init
cpu_init_cp15
The call to cpu_init_cp15() is performed in arch_cpu_init() when we
detect that we are the A9.
>
>>> Anyway: your patch moves cp15 init far enough that the AVP won't execute
>>> it,
>>> but the A9 will.
>>
>> Well, on Tegra we call the cp15 init directly when it is safe to do so,
>> later.
>>
>>>
>>> Only, what will happen when another multiple-core ARM SoC gets U-Boot
>>> support but the location you chose for cp15 init is inadequate for it?
>>> Shall
>>> we move cp15 init again, and where?
>>
>> If CONFIG_SKIP_LOWLEVEL_INIT is not defined, then the low level init
>> will operate exactly as now. My patch effectively just allows you to
>> have CONFIG_SKIP_LOWLEVEL_INIT but later call part of that lowlevel
>> init. In my view, the cp15 init should not be lumped in with the
>> 'memory init' or whatever else it expected to be done in the lowlevel
>> init.
>
> I think I get the point: due to the fact that both cores run the same starup
> code path, and due to the fact that they run it one after the other and
> share a lot of devices, at mots one of them shoud perform low level inits
> (e.g. RAM init), and some low-level inits can only be performed by one of
> them (e.g., cp15).
Well on Tegra the RAM init is down by the boot ROM. There is no low
level init bar the cp15 stuff.
>
>>> I'd prefer the cp15 init to stay where it is but execute only for A9, for
>>> instance by checking some core identification register.
>>
>> Well I actually haven't moved it! It is just that previously it was
>> impossible to call cp15_init from anywhere later.
>
> It is moved, in that it belongs to low level init... of A9.
OK, I see - you mean moved in order if not in source code file.
>
>> What you say can be done, it would involve some assembler though and
>> would need to be another CONFIG option. Was trying to avoid adding new
>> assembler.
>
> Low level init is about assembler and, well, low level. :)
Yes but it's yuck. Part of the clean-up is to remove most of the
assembler - really very little is needed.
>
> But I don't see why there should be another CONFIG option. IIUC, you should
> be able to do with only CONFIG_SKIP_LOWLEVEL_INIT: within the low level init
> code under it, you would do the equivalent of a switch, with one branch for
> AVM (and DDR init etc) and one branch for A9 (with cp15 init etc).
Yes I can, but I need to be able to call cp15_init. And I can't do
that because if CONFIG_SKIP_LOWLEVEL_INIT is defined, that code is
compiled out! So I need to move that cp15_init code outside the
CONFIG_SKIP_LOWLEVEL_INIT #ifdef. That is all I am trying to do,
honest!
Regards,
Simon
>
> Amicalement,
> --
> Albert.
>
^ permalink raw reply
* [U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits
From: Simon Glass @ 2011-10-22 4:58 UTC (permalink / raw)
To: u-boot
In-Reply-To: <4EA1F495.4010409@aribaud.net>
Hi Albert,
On Fri, Oct 21, 2011 at 3:39 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Le 22/10/2011 00:02, Simon Glass a ?crit :
>>
>> Hi Albert,
>>
>> On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD
>> <albert.u.boot@aribaud.net> ?wrote:
>>>
>>> Le 21/10/2011 23:12, Simon Glass a ?crit :
>>>>
>>>> Hi Albert,
>>>>
>>>> On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD
>>>> <albert.u.boot@aribaud.net> ? ?wrote:
>>>>>
>>>>> Le 21/10/2011 22:19, Simon Glass a ?crit :
>>>>>>
>>>>>> Hi Albert,
>>>>>>
>>>>>> On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD
>>>>>> <albert.u.boot@aribaud.net> ? ? ?wrote:
>>>>>>>
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> Le 10/10/2011 21:22, Simon Glass a ?crit :
>>>>>>>>
>>>>>>>> This brings a basic limits.h implementation into U-Boot.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Glass<sjg@chromium.org>
>>>>>>>> ---
>>>>>>>> ?fs/ubifs/ubifs.h | ? ?4 +---
>>>>>>>> ?include/limits.h | ? 40 ++++++++++++++++++++++++++++++++++++++++
>>>>>>>> ?2 files changed, 41 insertions(+), 3 deletions(-)
>>>>>>>> ?create mode 100644 include/limits.h
>>>>>>>
>>>>>>> Apparently, in all the U-Boot codebase, only one file required
>>>>>>> standard
>>>>>>> limit names, and gets them with three lines of code. Is it worth
>>>>>>> adding
>>>>>>> 40
>>>>>>> lines of code for this?
>>>>>>
>>>>>> Well 2/3 is the license header - and I thought it best to add all the
>>>>>> limits in one go. I can add just those few if you like.
>>>>>>
>>>>>> This file is used later in the patch series.
>>>>>
>>>>> I don't see much use of these in the subsequent patches either -- and
>>>>> those
>>>>> few uses could be discussed, such as for instance the use of INT_MAX as
>>>>> the
>>>>> maximum buffer size for some *printf() functions -- actually, anything
>>>>> very
>>>>> big would fit just as well, would it not?
>>>>
>>>> Yes it would, it's doesn't really need to be INT_MAX. Then again,
>>>> limits.h is a fairly standard file to have around, and INT_MAX is an
>>>> efficient 'large' value to load on many architectures.
>>>>
>>>> In any case it seems wrong to me that ubifs is defining its own
>>>> INT_MAX and U-Boot doesn't have one.
>>>
>>> My point is precisely that as standard as limits.h is, U-Boot has managed
>>> to
>>> survive not having it around so far, which kind of shows limits.h is not
>>> needed.
>>
>> By that logic one would never do any code clean ups. Do I understand
>> you correctly?
>
> You're extending my logic here: not all cleanups are done by adding a header
> file and replacing some lines by an include and some other lines. :)
>
> Actually, I don't think introducing limits.h is any cleanup; it is an
> attempt at using standards whenever possible, which may be fine with some
> standards: I'd be happy of U-Boot used uint{8,16,32}_t instead of
> u{8,16,32}, for instance, because it uses that a lot. With limits.h, my
> gripe with it here is that, while possible, I don't see it bringing benefits
> here as 1) the ubi code already defines what it needs, 2) other uses in the
> patchset do not actually require /limits/, and 3) there are not many places
> in the whole U-Boot code that do.
>
> If you can prove me wrong, i.e. if you can show that limits.h would add a
> lot of benefits to more than the other files in its own patchset, then I'll
> happily reconsider.
I see few and small benefits. Of course if it is not there then people
will not use it, so it is self-fulfilling.
>
>> But this is the least of my concerns :-) If you don't want it, don't
>> take it. Shall I modify the series to define its own INT_MAX, or just
>> chose a large number?
>
> Well I don't think the limits.h introduction is useful here, and not
> introducing it will barely cost a source code line. As for the number to use
> in *printf(), either way is fine with me, as this number is arbitrary
> anyway.
OK
>
>> BTW I think you are looking at the old version of that patch series -
>> we are now on v4. The limits.h patch is the same though. Later on in
>> the series I add comments to vsprintf() functions and move them into
>> their own header. If you apply the same logic there then that tidy is
>> not needed also. Please let me know.
>
> Thanks for reminding me. I did see the V4 series and it is the one I
> actually commented on in my previous mail. Apologies for not having made
> that explicit.
OK that's fine - I will redo the series without limits.h.
Regards,
Simon
>
>> Regards,
>> Simon
>
> Amicalement,
> --
> Albert.
>
^ permalink raw reply
* [U-Boot] [PATCH 8/8] tftpput: add tftpput command
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
This adds a command to start a tftp put transfer.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
common/cmd_net.c | 23 +++++++++++++++++++++++
include/config_cmd_all.h | 1 +
2 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/common/cmd_net.c b/common/cmd_net.c
index f610385..f89a24b 100644
--- a/common/cmd_net.c
+++ b/common/cmd_net.c
@@ -52,6 +52,22 @@ U_BOOT_CMD(
"[loadAddress] [[hostIPaddr:]bootfilename]"
);
+#ifdef CONFIG_CMD_TFTPPUT
+int do_tftpput(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+ int ret;
+
+ ret = netboot_common(TFTPPUT, cmdtp, argc, argv);
+ return ret;
+}
+
+U_BOOT_CMD(
+ tftpput, 4, 1, do_tftpput,
+ "TFTP put command, for uploading files to a server",
+ "Address Size [[hostIPaddr:]filename]"
+);
+#endif
+
#ifdef CONFIG_CMD_TFTPSRV
static int do_tftpsrv(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
{
@@ -203,6 +219,13 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
break;
+#ifdef CONFIG_CMD_TFTPPUT
+ case 4:
+ save_addr = strict_strtoul(argv[1], NULL, 16);
+ save_size = strict_strtoul(argv[2], NULL, 16);
+ copy_filename(BootFile, argv[3], sizeof(BootFile));
+ break;
+#endif
default:
show_boot_progress (-80);
return cmd_usage(cmdtp);
diff --git a/include/config_cmd_all.h b/include/config_cmd_all.h
index 9716f9c..7fa8661 100644
--- a/include/config_cmd_all.h
+++ b/include/config_cmd_all.h
@@ -82,6 +82,7 @@
#define CONFIG_CMD_SOURCE /* "source" command support */
#define CONFIG_CMD_SPI /* SPI utility */
#define CONFIG_CMD_TERMINAL /* built-in Serial Terminal */
+#define CONFIG_CMD_TFTPPUT /* TFTP upload */
#define CONFIG_CMD_UBI /* UBI Support */
#define CONFIG_CMD_UBIFS /* UBIFS Support */
#define CONFIG_CMD_UNIVERSE /* Tundra Universe Support */
--
1.7.3.1
^ permalink raw reply related
* [U-Boot] [PATCH 7/8] tftpput: implement tftp logic
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
This adds logic to tftp.c to implement the tftp 'put' command, and
updates the README.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
README | 2 +
net/net.c | 4 ++
net/tftp.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++----------
3 files changed, 100 insertions(+), 20 deletions(-)
diff --git a/README b/README
index 7e032a9..8261bfb 100644
--- a/README
+++ b/README
@@ -784,6 +784,7 @@ The following options need to be configured:
CONFIG_CMD_SOURCE "source" command Support
CONFIG_CMD_SPI * SPI serial bus support
CONFIG_CMD_TFTPSRV * TFTP transfer in server mode
+ CONFIG_CMD_TFTPPUT * TFTP put command (upload)
CONFIG_CMD_TIME * run command and report execution time
CONFIG_CMD_USB * USB support
CONFIG_CMD_CDP * Cisco Discover Protocol support
@@ -3340,6 +3341,7 @@ bootp - boot image via network using BootP/TFTP protocol
tftpboot- boot image via network using TFTP protocol
and env variables "ipaddr" and "serverip"
(and eventually "gatewayip")
+tftpput - upload a file via network using TFTP protocol
rarpboot- boot image via network using RARP/TFTP protocol
diskboot- boot from IDE devicebootd - boot default, i.e., run 'bootcmd'
loads - load S-Record file over serial line
diff --git a/net/net.c b/net/net.c
index ce07ed6..89596d3 100644
--- a/net/net.c
+++ b/net/net.c
@@ -406,6 +406,9 @@ restart:
NetBootFileXferSize = 0;
switch (protocol) {
case TFTPGET:
+#ifdef CONFIG_CMD_TFTPPUT
+ case TFTPPUT:
+#endif
/* always use ARP to get server ethernet address */
TftpStart(protocol);
break;
@@ -1758,6 +1761,7 @@ static int net_check_prereq(enum proto_t protocol)
case NFS:
#endif
case TFTPGET:
+ case TFTPPUT:
if (NetServerIP == 0) {
puts("*** ERROR: `serverip' not set\n");
return 1;
diff --git a/net/tftp.c b/net/tftp.c
index 3a58e32..02862c6 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -82,6 +82,11 @@ static int TftpTsize;
static short TftpNumchars;
#endif
static int TftpWriting; /* 1 if writing, else 0 */
+#ifdef CONFIG_CMD_TFTPPUT
+static int TftpFinalBlock; /* 1 if we have sent the last block */
+#else
+#define TftpWriting 0
+#endif
#define STATE_SEND_RRQ 1
#define STATE_DATA 2
@@ -89,6 +94,7 @@ static int TftpWriting; /* 1 if writing, else 0 */
#define STATE_BAD_MAGIC 4
#define STATE_OACK 5
#define STATE_RECV_WRQ 6
+#define STATE_SEND_WRQ 7
/* default TFTP block size */
#define TFTP_BLOCK_SIZE 512
@@ -191,6 +197,28 @@ store_block(unsigned block, uchar *src, unsigned len)
NetBootFileXferSize = newsize;
}
+#ifdef CONFIG_CMD_TFTPPUT
+/**
+ * Load the next block from memory to be sent over tftp.
+ *
+ * @param block Block number to send
+ * @param dst Destination buffer for data
+ * @param len Number of bytes in block (this one and every other)
+ * @return number of bytes loaded
+ */
+static int load_block(unsigned block, uchar *dst, unsigned len)
+{
+ ulong offset = (block - 1) * len + TftpBlockWrapOffset;
+ ulong tosend = len;
+
+ tosend = min(NetBootFileXferSize - offset, tosend);
+ (void)memcpy(dst, (void *)(save_addr + offset), tosend);
+ debug("%s: block=%d, offset=%ld, len=%d, tosend=%ld\n", __func__,
+ block, offset, len, tosend);
+ return tosend;
+}
+#endif
+
static void TftpSend(void);
static void TftpTimeout(void);
@@ -235,7 +263,7 @@ static void tftp_complete(void)
static void
TftpSend(void)
{
- volatile uchar *pkt;
+ uchar *pkt;
volatile uchar *xp;
int len = 0;
volatile ushort *s;
@@ -251,14 +279,15 @@ TftpSend(void)
* We will always be sending some sort of packet, so
* cobble together the packet headers now.
*/
- pkt = NetTxPacket + NetEthHdrSize() + IP_HDR_SIZE;
+ pkt = (uchar *)(NetTxPacket + NetEthHdrSize() + IP_HDR_SIZE);
switch (TftpState) {
-
case STATE_SEND_RRQ:
+ case STATE_SEND_WRQ:
xp = pkt;
s = (ushort *)pkt;
- *s++ = htons(TFTP_RRQ);
+ *s++ = htons(TftpState == STATE_SEND_RRQ ? TFTP_RRQ
+ : TFTP_WRQ);
pkt = (uchar *)s;
strcpy((char *)pkt, tftp_filename);
pkt += strlen(tftp_filename) + 1;
@@ -270,8 +299,8 @@ TftpSend(void)
debug("send option \"timeout %s\"\n", (char *)pkt);
pkt += strlen((char *)pkt) + 1;
#ifdef CONFIG_TFTP_TSIZE
- memcpy((char *)pkt, "tsize\0000\0", 8);
- pkt += 8;
+ pkt += sprintf((char *)pkt, "tsize%c%lu%c",
+ 0, NetBootFileXferSize, 0);
#endif
/* try for more effic. blk size */
pkt += sprintf((char *)pkt, "blksize%c%d%c",
@@ -302,9 +331,19 @@ TftpSend(void)
case STATE_DATA:
xp = pkt;
s = (ushort *)pkt;
- *s++ = htons(TFTP_ACK);
- *s++ = htons(TftpBlock);
- pkt = (uchar *)s;
+ s[0] = htons(TFTP_ACK);
+ s[1] = htons(TftpBlock);
+ pkt = (uchar *)(s + 2);
+#ifdef CONFIG_CMD_TFTPPUT
+ if (TftpWriting) {
+ int toload = TftpBlkSize;
+ int loaded = load_block(TftpBlock, pkt, toload);
+
+ s[0] = htons(TFTP_DATA);
+ pkt += loaded;
+ TftpFinalBlock = (loaded < toload);
+ }
+#endif
len = pkt - xp;
break;
@@ -312,7 +351,8 @@ TftpSend(void)
xp = pkt;
s = (ushort *)pkt;
*s++ = htons(TFTP_ERROR);
- *s++ = htons(3);
+ *s++ = htons(3);
+
pkt = (uchar *)s;
strcpy((char *)pkt, "File too large");
pkt += 14 /*strlen("File too large")*/ + 1;
@@ -342,7 +382,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
{
ushort proto;
ushort *s;
- int i;
+ int i, block;
if (dest != TftpOurPort) {
#ifdef CONFIG_MCAST_TFTP
@@ -352,7 +392,7 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
return;
}
if (TftpState != STATE_SEND_RRQ && src != TftpRemotePort &&
- TftpState != STATE_RECV_WRQ)
+ TftpState != STATE_RECV_WRQ && TftpState != STATE_SEND_WRQ)
return;
if (len < 2)
@@ -362,11 +402,28 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
s = (ushort *)pkt;
proto = *s++;
pkt = (uchar *)s;
+ block = ntohs(*s);
switch (ntohs(proto)) {
case TFTP_RRQ:
+ break;
+
case TFTP_ACK:
+#ifdef CONFIG_CMD_TFTPPUT
+ debug("Ack block %d\n", block);
+ show_block_marker();
+ if (TftpWriting) {
+ if (TftpFinalBlock) {
+ tftp_complete();
+ } else {
+ TftpBlock = block + 1;
+ show_block_marker();
+ TftpSend(); /* Send next data block */
+ }
+ }
+#endif
break;
+
default:
break;
@@ -417,7 +474,14 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
TftpState = STATE_DATA; /* passive.. */
else
#endif
- TftpSend(); /* Send ACK */
+#ifdef CONFIG_CMD_TFTPPUT
+ if (TftpWriting) {
+ /* Get ready to send the first block */
+ TftpState = STATE_DATA;
+ TftpBlock++;
+ }
+#endif
+ TftpSend(); /* Send ACK or first data block */
break;
case TFTP_DATA:
if (len < 2)
@@ -626,8 +690,8 @@ void TftpStart(proto_t protocol)
}
printf("Using %s device\n", eth_get_name());
- printf("TFTP from server %pI4"
- "; our IP address is %pI4", &TftpRemoteIP, &NetOurIP);
+ printf("TFTP %s server %pI4; our IP address is %pI4",
+ protocol == TFTPPUT ? "to" : "from", &TftpRemoteIP, &NetOurIP);
/* Check if we need to send across this subnet */
if (NetOurGatewayIP && NetOurSubnetMask) {
@@ -649,10 +713,21 @@ void TftpStart(proto_t protocol)
putc('\n');
TftpWriting = (protocol == TFTPPUT);
-
- printf("Load address: 0x%lx\n", load_addr);
-
- puts("Loading: *\b");
+#ifdef CONFIG_CMD_TFTPPUT
+ if (TftpWriting) {
+ printf("Save address: 0x%lx\n", save_addr);
+ printf("Save size: 0x%lx\n", save_size);
+ NetBootFileXferSize = save_size;
+ puts("Saving: *\b");
+ TftpState = STATE_SEND_WRQ;
+ TftpFinalBlock = 0;
+ }
+#endif
+ else {
+ printf("Load address: 0x%lx\n", load_addr);
+ puts("Loading: *\b");
+ TftpState = STATE_SEND_RRQ;
+ }
TftpTimeoutCountMax = TftpRRQTimeoutCountMax;
@@ -661,7 +736,6 @@ void TftpStart(proto_t protocol)
TftpRemotePort = WELL_KNOWN_PORT;
TftpTimeoutCount = 0;
- TftpState = STATE_SEND_RRQ;
/* Use a pseudo-random port unless a specific port is set */
TftpOurPort = 1024 + (get_timer(0) % 3072);
--
1.7.3.1
^ permalink raw reply related
* [U-Boot] [PATCH 6/8] tftpput: add save_addr and save_size global variables
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
We need something akin to load_addr to handle saving data.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
common/cmd_nvedit.c | 2 ++
include/common.h | 2 ++
2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index da7028c..30cf8df 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -79,6 +79,8 @@ SPI_FLASH|MG_DISK|NVRAM|MMC} or CONFIG_ENV_IS_NOWHERE
#define MAX_ENV_SIZE (1 << 20) /* 1 MiB */
ulong load_addr = CONFIG_SYS_LOAD_ADDR; /* Default Load Address */
+ulong save_addr; /* Default Save Address */
+ulong save_size; /* Default Save Size (in bytes) */
/*
* Table with supported baudrates (defined in config_xyz.h)
diff --git a/include/common.h b/include/common.h
index 99c2a37..d7382d9 100644
--- a/include/common.h
+++ b/include/common.h
@@ -278,6 +278,8 @@ void flash_perror (int);
int source (ulong addr, const char *fit_uname);
extern ulong load_addr; /* Default Load Address */
+extern ulong save_addr; /* Default Save Address */
+extern ulong save_size; /* Default Save Size */
/* common/cmd_doc.c */
void doc_probe(unsigned long physadr);
--
1.7.3.1
^ permalink raw reply related
* [U-Boot] [PATCH 5/8] tftpput: support selecting get/put for tftp
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
TftpStart should support starting either a get or a put.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
include/net.h | 4 ++--
net/bootp.c | 2 +-
net/tftp.c | 5 +++--
net/tftp.h | 2 +-
4 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/net.h b/include/net.h
index 3dfe073..2e5a898 100644
--- a/include/net.h
+++ b/include/net.h
@@ -349,8 +349,8 @@ extern int NetRestartWrap; /* Tried all network devices */
enum {
BOOTP, RARP, ARP, TFTPGET, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,
- TFTPSRV
-};
+ TFTPSRV, TFTPPUT
+} proto_t;
/* from net/net.c */
extern char BootFile[128]; /* Boot File name */
diff --git a/net/bootp.c b/net/bootp.c
index a003c42..068b037 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -165,7 +165,7 @@ static void auto_load(void)
}
#endif
}
- TftpStart();
+ TftpStart(TFTPGET);
}
#if !defined(CONFIG_CMD_DHCP)
diff --git a/net/tftp.c b/net/tftp.c
index 4ad85a5..3a58e32 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -81,6 +81,7 @@ static int TftpTsize;
/* The number of hashes we printed */
static short TftpNumchars;
#endif
+static int TftpWriting; /* 1 if writing, else 0 */
#define STATE_SEND_RRQ 1
#define STATE_DATA 2
@@ -572,8 +573,7 @@ TftpTimeout(void)
}
-void
-TftpStart(void)
+void TftpStart(proto_t protocol)
{
char *ep; /* Environment pointer */
@@ -648,6 +648,7 @@ TftpStart(void)
}
putc('\n');
+ TftpWriting = (protocol == TFTPPUT);
printf("Load address: 0x%lx\n", load_addr);
diff --git a/net/tftp.h b/net/tftp.h
index 3abdf7b..2d5d594 100644
--- a/net/tftp.h
+++ b/net/tftp.h
@@ -16,7 +16,7 @@
*/
/* tftp.c */
-extern void TftpStart (void); /* Begin TFTP get */
+void TftpStart(proto_t protocol); /* Begin TFTP get/put */
#ifdef CONFIG_CMD_TFTPSRV
extern void TftpStartServer(void); /* Wait for incoming TFTP put */
--
1.7.3.1
^ permalink raw reply related
* [U-Boot] [PATCH 4/8] tftpput: move common code into separate functions
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
We want to show block markers on completion of get and put, so
move this common code into separate functions.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
net/tftp.c | 70 ++++++++++++++++++++++++++++++++++-------------------------
1 files changed, 40 insertions(+), 30 deletions(-)
diff --git a/net/tftp.c b/net/tftp.c
index da8eeaa..4ad85a5 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -195,6 +195,42 @@ static void TftpTimeout(void);
/**********************************************************************/
+static void show_block_marker(void)
+{
+ ulong pos = TftpBlock * TftpBlkSize + TftpBlockWrapOffset;
+
+#ifdef CONFIG_TFTP_TSIZE
+ if (TftpTsize) {
+ while (TftpNumchars <
+ /* TODO: needs to be different for put */
+ pos * 50 / TftpTsize) {
+ putc('#');
+ TftpNumchars++;
+ }
+ }
+#endif
+ else {
+ if (((TftpBlock - 1) % 10) == 0)
+ putc('#');
+ else if ((TftpBlock % (10 * HASHES_PER_LINE)) == 0)
+ puts("\n\t ");
+ }
+}
+
+/* The TFTP get or put is complete */
+static void tftp_complete(void)
+{
+#ifdef CONFIG_TFTP_TSIZE
+ /* Print hash marks for the last packet received */
+ while (TftpTsize && TftpNumchars < 49) {
+ putc('#');
+ TftpNumchars++;
+ }
+#endif
+ puts("\ndone\n");
+ NetState = NETLOOP_SUCCESS;
+}
+
static void
TftpSend(void)
{
@@ -400,21 +436,8 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
TftpBlkSize * TFTP_SEQUENCE_SIZE;
printf("\n\t %lu MB received\n\t ",
TftpBlockWrapOffset>>20);
- }
-#ifdef CONFIG_TFTP_TSIZE
- else if (TftpTsize) {
- while (TftpNumchars <
- NetBootFileXferSize * 50 / TftpTsize) {
- putc('#');
- TftpNumchars++;
- }
- }
-#endif
- else {
- if (((TftpBlock - 1) % 10) == 0)
- putc('#');
- else if ((TftpBlock % (10 * HASHES_PER_LINE)) == 0)
- puts("\n\t ");
+ } else {
+ show_block_marker();
}
if (TftpState == STATE_SEND_RRQ)
@@ -498,21 +521,8 @@ TftpHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
}
else
#endif
- if (len < TftpBlkSize) {
- /*
- * We received the whole thing. Try to
- * run it.
- */
-#ifdef CONFIG_TFTP_TSIZE
- /* Print hash marks for the last packet received */
- while (TftpTsize && TftpNumchars < 49) {
- putc('#');
- TftpNumchars++;
- }
-#endif
- puts("\ndone\n");
- NetState = NETLOOP_SUCCESS;
- }
+ if (len < TftpBlkSize)
+ tftp_complete();
break;
case TFTP_ERROR:
--
1.7.3.1
^ permalink raw reply related
* [U-Boot] [PATCH 3/8] tftpput: Rename TFTP to TFTPGET
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
This is a better name for this protocol. Also remove the typedef to keep
checkpatch happy, and move zeroing of NetBootFileXferSize a little
earlier since TFTPPUT will need to change this.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
common/cmd_net.c | 8 ++++----
include/net.h | 8 +++++---
net/net.c | 18 ++++++++----------
3 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/common/cmd_net.c b/common/cmd_net.c
index 872f4a6..f610385 100644
--- a/common/cmd_net.c
+++ b/common/cmd_net.c
@@ -28,7 +28,7 @@
#include <command.h>
#include <net.h>
-static int netboot_common (proto_t, cmd_tbl_t *, int , char * const []);
+static int netboot_common(enum proto_t, cmd_tbl_t *, int, char * const []);
int do_bootp (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
{
@@ -43,7 +43,7 @@ U_BOOT_CMD(
int do_tftpb (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
{
- return netboot_common (TFTP, cmdtp, argc, argv);
+ return netboot_common(TFTPGET, cmdtp, argc, argv);
}
U_BOOT_CMD(
@@ -167,8 +167,8 @@ static void netboot_update_env (void)
#endif
}
-static int
-netboot_common (proto_t proto, cmd_tbl_t *cmdtp, int argc, char * const argv[])
+static int netboot_common(enum proto_t proto, cmd_tbl_t *cmdtp, int argc,
+ char * const argv[])
{
char *s;
char *end;
diff --git a/include/net.h b/include/net.h
index d5d37b6..3dfe073 100644
--- a/include/net.h
+++ b/include/net.h
@@ -347,8 +347,10 @@ extern int NetState; /* Network loop state */
extern int NetRestartWrap; /* Tried all network devices */
-typedef enum { BOOTP, RARP, ARP, TFTP, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,
- TFTPSRV } proto_t;
+enum {
+ BOOTP, RARP, ARP, TFTPGET, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,
+ TFTPSRV
+};
/* from net/net.c */
extern char BootFile[128]; /* Boot File name */
@@ -374,7 +376,7 @@ extern int NetTimeOffset; /* offset time from UTC */
#endif
/* Initialize the network adapter */
-extern int NetLoop(proto_t);
+extern int NetLoop(enum proto_t);
/* Shutdown adapters and cleanup */
extern void NetStop(void);
diff --git a/net/net.c b/net/net.c
index 5e67886..ce07ed6 100644
--- a/net/net.c
+++ b/net/net.c
@@ -224,7 +224,7 @@ static ulong timeDelta;
/* THE transmit packet */
volatile uchar *NetTxPacket;
-static int net_check_prereq(proto_t protocol);
+static int net_check_prereq(enum proto_t protocol);
static int NetTryCount;
@@ -310,8 +310,7 @@ void ArpTimeoutCheck(void)
}
}
-static void
-NetInitLoop(proto_t protocol)
+static void NetInitLoop(enum proto_t protocol)
{
static int env_changed_id;
bd_t *bd = gd->bd;
@@ -340,8 +339,7 @@ NetInitLoop(proto_t protocol)
* Main network processing loop.
*/
-int
-NetLoop(proto_t protocol)
+int NetLoop(enum proto_t protocol)
{
bd_t *bd = gd->bd;
@@ -405,10 +403,11 @@ restart:
case 0:
NetDevExists = 1;
+ NetBootFileXferSize = 0;
switch (protocol) {
- case TFTP:
+ case TFTPGET:
/* always use ARP to get server ethernet address */
- TftpStart();
+ TftpStart(protocol);
break;
#ifdef CONFIG_CMD_TFTPSRV
case TFTPSRV:
@@ -470,7 +469,6 @@ restart:
break;
}
- NetBootFileXferSize = 0;
break;
}
@@ -1728,7 +1726,7 @@ NetReceive(volatile uchar *inpkt, int len)
/**********************************************************************/
-static int net_check_prereq(proto_t protocol)
+static int net_check_prereq(enum proto_t protocol)
{
switch (protocol) {
/* Fall through */
@@ -1759,7 +1757,7 @@ static int net_check_prereq(proto_t protocol)
#if defined(CONFIG_CMD_NFS)
case NFS:
#endif
- case TFTP:
+ case TFTPGET:
if (NetServerIP == 0) {
puts("*** ERROR: `serverip' not set\n");
return 1;
--
1.7.3.1
^ permalink raw reply related
* [U-Boot] [PATCH 2/8] Add setenv_uint() and setenv_addr()
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
It seems we put numbers and addresses into environment variables a lot.
We should have some functions to do this.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
common/cmd_nvedit.c | 29 +++++++++++++++++++++++++++++
include/common.h | 2 ++
2 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 101bc49..da7028c 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -377,6 +377,35 @@ int setenv(const char *varname, const char *varvalue)
return _do_env_set(0, 3, (char * const *)argv);
}
+/**
+ * Set an environment variable to an integer value
+ *
+ * @param varname Environmet variable to set
+ * @param value Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int setenv_ulong(const char *varname, ulong value)
+{
+ char *str = simple_itoa(value);
+
+ return setenv(varname, str);
+}
+
+/**
+ * Set an environment variable to an address in hex
+ *
+ * @param varname Environmet variable to set
+ * @param addr Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int setenv_addr(const char *varname, const void *addr)
+{
+ char str[17];
+
+ sprintf(str, "%x", (uintptr_t)addr);
+ return setenv(varname, str);
+}
+
int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
{
if (argc < 2)
diff --git a/include/common.h b/include/common.h
index 55f772d..99c2a37 100644
--- a/include/common.h
+++ b/include/common.h
@@ -294,6 +294,8 @@ int inline setenv (const char *, const char *);
#else
int setenv (const char *, const char *);
#endif /* CONFIG_PPC */
+int setenv_ulong(const char *varname, ulong value);
+int setenv_addr(const char *varname, const void *addr);
#ifdef CONFIG_ARM
# include <asm/mach-types.h>
# include <asm/setup.h>
--
1.7.3.1
^ permalink raw reply related
* [U-Boot] [PATCH 1/8] Move simple_itoa to vsprintf
From: Simon Glass @ 2011-10-22 4:51 UTC (permalink / raw)
To: u-boot
In-Reply-To: <1319259100-11376-1-git-send-email-sjg@chromium.org>
This function is generally useful and shouldn't hide away in hush. It
has been moved as is.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
common/hush.c | 15 ---------------
include/common.h | 1 +
lib/vsprintf.c | 16 ++++++++++++++++
3 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/common/hush.c b/common/hush.c
index 85a6030..9bdb499 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -17,7 +17,6 @@
* Erik W. Troan, which they placed in the public domain. I don't know
* how much of the Johnson/Troan code has survived the repeated rewrites.
* Other credits:
- * simple_itoa() was lifted from boa-0.93.15
* b_addchr() derived from similar w_addchar function in glibc-2.2
* setup_redirect(), redirect_opt_num(), and big chunks of main()
* and many builtins derived from contributions by Erik Andersen
@@ -922,20 +921,6 @@ static int b_addqchr(o_string *o, int ch, int quote)
return b_addchr(o, ch);
}
-/* belongs in utility.c */
-char *simple_itoa(unsigned int i)
-{
- /* 21 digits plus null terminator, good for 64-bit or smaller ints */
- static char local[22];
- char *p = &local[21];
- *p-- = '\0';
- do {
- *p-- = '0' + i % 10;
- i /= 10;
- } while (i > 0);
- return p + 1;
-}
-
#ifndef __U_BOOT__
static int b_adduint(o_string *o, unsigned int i)
{
diff --git a/include/common.h b/include/common.h
index eb19a44..55f772d 100644
--- a/include/common.h
+++ b/include/common.h
@@ -702,6 +702,7 @@ void panic(const char *fmt, ...)
int sprintf(char * buf, const char *fmt, ...)
__attribute__ ((format (__printf__, 2, 3)));
int vsprintf(char *buf, const char *fmt, va_list args);
+char *simple_itoa(ulong i);
/* lib/strmhz.c */
char * strmhz(char *buf, unsigned long hz);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 79dead3..e497a86 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -7,6 +7,8 @@
/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
/*
* Wirzenius wrote this portably, Torvalds fucked it up :-)
+ *
+ * from hush: simple_itoa() was lifted from boa-0.93.15
*/
#include <stdarg.h>
@@ -738,3 +740,17 @@ void __assert_fail(const char *assertion, const char *file, unsigned line,
panic("%s:%u: %s: Assertion `%s' failed.", file, line, function,
assertion);
}
+
+char *simple_itoa(ulong i)
+{
+ /* 21 digits plus null terminator, good for 64-bit or smaller ints */
+ static char local[22];
+ char *p = &local[21];
+
+ *p-- = '\0';
+ do {
+ *p-- = '0' + i % 10;
+ i /= 10;
+ } while (i > 0);
+ return p + 1;
+}
--
1.7.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.