[PATCH] MEDIA CODA: Fix NULL ptr dereference in the encoder.

[Krzysztof Halasa]

ctx->mb_err_cnt_ctrl could be NULL in case of failed initialization
(on decoders), and encoders don't use it at all.

Fixes: b2d3bef1aa78 ("media: coda: Add a V4L2 user for control error 
macroblocks count")
Signed-off-by: Krzysztof Halasa 
Cc: sta...@vger.kernel.org # 5.11+

diff --git a/drivers/media/platform/coda/coda-bit.c 
b/drivers/media/platform/coda/coda-bit.c
index 2f42808c43a4..26e37cbfe8dd 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2373,8 +2373,10 @@ static void coda_finish_decode(struct coda_ctx *ctx)
if (err_mb > 0) {
if (__ratelimit(>mb_err_rs))
coda_dbg(1, ctx, "errors in %d macroblocks\n", err_mb);
-   v4l2_ctrl_s_ctrl(ctx->mb_err_cnt_ctrl,
-v4l2_ctrl_g_ctrl(ctx->mb_err_cnt_ctrl) + 
err_mb);
+   if (ctx->mb_err_cnt_ctrl)
+   v4l2_ctrl_s_ctrl(ctx->mb_err_cnt_ctrl,
+v4l2_ctrl_g_ctrl(ctx->mb_err_cnt_ctrl)
++ err_mb);
}
 
if (dev->devtype->product == CODA_HX4 ||
diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 96802b8f47ea..285c80f87b65 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2062,7 +2062,8 @@ static int coda_start_streaming(struct vb2_queue *q, 
unsigned int count)
if (q_data_dst->fourcc == V4L2_PIX_FMT_JPEG)
ctx->params.gop_size = 1;
ctx->gopcounter = ctx->params.gop_size - 1;
-   v4l2_ctrl_s_ctrl(ctx->mb_err_cnt_ctrl, 0);
+   if (ctx->mb_err_cnt_ctrl)
+   v4l2_ctrl_s_ctrl(ctx->mb_err_cnt_ctrl, 0);
 
ret = ctx->ops->start_streaming(ctx);
if (ctx->inst_type == CODA_INST_DECODER) {

-- 
Krzysztof Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

[PATCH] Marvell Sky2 Ethernet adapter: fix warning messages.

[Krzysztof Halasa]

sky2.c driver uses netdev_warn() before the net device is initialized.
Fix it by using dev_warn() instead.

Signed-off-by: Krzysztof Halasa 

--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -4806,12 +4806,11 @@ static struct net_device *sky2_init_netdev(struct 
sky2_hw *hw, unsigned port,
if (!is_valid_ether_addr(dev->dev_addr)) {
struct sockaddr sa = { AF_UNSPEC };
 
-   netdev_warn(dev,
-   "Invalid MAC address, defaulting to random\n");
+   dev_warn(>pdev->dev, "Invalid MAC address, defaulting to 
random\n");
eth_hw_addr_random(dev);
memcpy(sa.sa_data, dev->dev_addr, ETH_ALEN);
if (sky2_set_mac_address(dev, ))
-   netdev_warn(dev, "Failed to set MAC address.\n");
+   dev_warn(>pdev->dev, "Failed to set MAC 
address.\n");
}
 
return dev;

-- 
Krzysztof Halasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Krzysztof Halasa]

"Luis R. Rodriguez" <mcg...@kernel.org> writes:

> So we can nuke CONFIG_WANXL_BUILD_FIRMWARE now?

I'm uncertain I understand why do you want it, or maybe what are you
trying to do at all.

And what use would wanxlfw.S (the assembly source) have if the option is
removed?

>> It's more about delivering the .S source for the firmware, I guess.
>> Nobody is expected to build it. The fw is about 2.5 KB and is directly
>> linked with the driver.
>
> :P Future work I guess would be to just use the firmware API and stuff
> it into linux-firmware?

Who's going to make it happen?
The last time I checked (several years ago), wanXL worked. Who's going
to test it after the change?

I assume linux-firmware could include fw source and there would be means
to build the binary.

Just to be sure: the wanXL firmware has exactly nothing to do with FW
loader, nothing depends on it (nor the other way around), it's just
(with the rest of the wanXL code) an old piece of a driver for an old
card.

The question is, what do we gain by messing with it?
-- 
Krzysztof Halasa

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Krzysztof Halasa]

"Luis R. Rodriguez"  writes:

> So we can nuke CONFIG_WANXL_BUILD_FIRMWARE now?

I'm uncertain I understand why do you want it, or maybe what are you
trying to do at all.

And what use would wanxlfw.S (the assembly source) have if the option is
removed?

>> It's more about delivering the .S source for the firmware, I guess.
>> Nobody is expected to build it. The fw is about 2.5 KB and is directly
>> linked with the driver.
>
> :P Future work I guess would be to just use the firmware API and stuff
> it into linux-firmware?

Who's going to make it happen?
The last time I checked (several years ago), wanXL worked. Who's going
to test it after the change?

I assume linux-firmware could include fw source and there would be means
to build the binary.

Just to be sure: the wanXL firmware has exactly nothing to do with FW
loader, nothing depends on it (nor the other way around), it's just
(with the rest of the wanXL code) an old piece of a driver for an old
card.

The question is, what do we gain by messing with it?
-- 
Krzysztof Halasa

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Krzysztof Halasa]

"Luis R. Rodriguez" <mcg...@kernel.org> writes:

>   * CONFIG_WANXL --> CONFIG_WANXL_BUILD_FIRMWARE
>   * CONFIG_SCSI_AIC79XX --> CONFIG_AIC79XX_BUILD_FIRMWARE
>
> To this day both of these drivers are building driver *firmwares* when
> the option CONFIG_PREVENT_FIRMWARE_BUILD is disabled, and they don't
> even make use of the firmware API at all.

Don't know for sure about Adaptec, but wanXL firmware fits every
definition of "stable". BTW it's a 1997 or so early PCI card, built
around the PLX PCI9060 bus mastering PCI bridge and Motorola 68360
(the original QUICC) processor. Maximum bit rate of 2 Mb/s on each sync
serial port.

It's more about delivering the .S source for the firmware, I guess.
Nobody is expected to build it. The fw is about 2.5 KB and is directly
linked with the driver.
-- 
Krzysztof Halasa

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Krzysztof Halasa]

"Luis R. Rodriguez"  writes:

>   * CONFIG_WANXL --> CONFIG_WANXL_BUILD_FIRMWARE
>   * CONFIG_SCSI_AIC79XX --> CONFIG_AIC79XX_BUILD_FIRMWARE
>
> To this day both of these drivers are building driver *firmwares* when
> the option CONFIG_PREVENT_FIRMWARE_BUILD is disabled, and they don't
> even make use of the firmware API at all.

Don't know for sure about Adaptec, but wanXL firmware fits every
definition of "stable". BTW it's a 1997 or so early PCI card, built
around the PLX PCI9060 bus mastering PCI bridge and Motorola 68360
(the original QUICC) processor. Maximum bit rate of 2 Mb/s on each sync
serial port.

It's more about delivering the .S source for the firmware, I guess.
Nobody is expected to build it. The fw is about 2.5 KB and is directly
linked with the driver.
-- 
Krzysztof Halasa

Re: [PATCH] Carrier detect ok, don't turn off negotiation

[Krzysztof Halasa]

Denis Du <dudenis2...@yahoo.ca> writes:

>>From the above code, I can get that only Carrier have some change, it
> will restart the protocol by hdlc_proto_start(dev);and thus the timer,
> the previous timer expired due to protocol fail.
>
> If carrier keep no change by if (hdlc->carrier == on)
> goto carrier_exit; /* no change in DCD line level */It will do
> nothing, not start any new protocol and thus the timer.

Sorry about being late, just returned home and am trying to get all the
backlogs under control.

I remember the PPP standard is a bit cloudy about the possible issue,
but the latter indeed exists (the PPP state machine was written directly
to STD-51). There is related (more visible in practice, though we aren't
affected) issue of "active" vs "passive" mode (hdlc_ppp.c is "active",
and two "passives" wouldn't negotiate at all).

Anyway the problem is real (though not very visible in practice,
especially on relatively modern links rather than 300 or 1200 bps dialup
connections) and should be fixed. Looking at the patch, my first
impression is it makes the code differ from STD-51 a little bit.
On the other hand, perhaps applying it as is and forgetting about the
issue is the way to go.

Ideally, I think the negotiation failure should end up (optionally, in
addition to the current behavior) in some configurable sleep, then
the negotiation should restart. If it's worth the effort at this point,
I don't know.

Perhaps I could look at this later, but no promises (this requires
pulling on and setting up some legacy hardware).

Anyway, since the patch is safe and can solve an existing problem:

Acked-by: Krzysztof Halasa <k...@pm.waw.pl>
-- 
Krzysztof Halasa

Re: [PATCH] Carrier detect ok, don't turn off negotiation

[Krzysztof Halasa]

Denis Du  writes:

>>From the above code, I can get that only Carrier have some change, it
> will restart the protocol by hdlc_proto_start(dev);and thus the timer,
> the previous timer expired due to protocol fail.
>
> If carrier keep no change by if (hdlc->carrier == on)
> goto carrier_exit; /* no change in DCD line level */It will do
> nothing, not start any new protocol and thus the timer.

Sorry about being late, just returned home and am trying to get all the
backlogs under control.

I remember the PPP standard is a bit cloudy about the possible issue,
but the latter indeed exists (the PPP state machine was written directly
to STD-51). There is related (more visible in practice, though we aren't
affected) issue of "active" vs "passive" mode (hdlc_ppp.c is "active",
and two "passives" wouldn't negotiate at all).

Anyway the problem is real (though not very visible in practice,
especially on relatively modern links rather than 300 or 1200 bps dialup
connections) and should be fixed. Looking at the patch, my first
impression is it makes the code differ from STD-51 a little bit.
On the other hand, perhaps applying it as is and forgetting about the
issue is the way to go.

Ideally, I think the negotiation failure should end up (optionally, in
addition to the current behavior) in some configurable sleep, then
the negotiation should restart. If it's worth the effort at this point,
I don't know.

Perhaps I could look at this later, but no promises (this requires
pulling on and setting up some legacy hardware).

Anyway, since the patch is safe and can solve an existing problem:

Acked-by: Krzysztof Halasa 
-- 
Krzysztof Halasa

Re: [PATCH 13/31] timer: Remove meaningless .data/.function assignments

[Krzysztof Halasa]

Kees Cook <keesc...@chromium.org> writes:

> Several timer users needlessly reset their .function/.data fields during
> their timer callback, but nothing else changes them. Some users do not
> use their .data field at all. Each instance is removed here.

For *wan/hdlc*
Acked-by: Krzysztof Halasa <k...@pm.waw.pl>

> --- a/drivers/net/wan/hdlc_cisco.c
> +++ b/drivers/net/wan/hdlc_cisco.c
> @@ -276,8 +276,6 @@ static void cisco_timer(unsigned long arg)
>   spin_unlock(>lock);
>  
>   st->timer.expires = jiffies + st->settings.interval * HZ;
> - st->timer.function = cisco_timer;
> - st->timer.data = arg;
>   add_timer(>timer);
>  }
>  
> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index de42faca076a..7da2424c28a4 100644
> --- a/drivers/net/wan/hdlc_fr.c
> +++ b/drivers/net/wan/hdlc_fr.c
> @@ -644,8 +644,6 @@ static void fr_timer(unsigned long arg)
>   state(hdlc)->settings.t391 * HZ;
>   }
>  
> - state(hdlc)->timer.function = fr_timer;
> - state(hdlc)->timer.data = arg;
>   add_timer((hdlc)->timer);
>  }

-- 
Krzysztof Halasa

Re: [PATCH 13/31] timer: Remove meaningless .data/.function assignments

[Krzysztof Halasa]

Kees Cook  writes:

> Several timer users needlessly reset their .function/.data fields during
> their timer callback, but nothing else changes them. Some users do not
> use their .data field at all. Each instance is removed here.

For *wan/hdlc*
Acked-by: Krzysztof Halasa 

> --- a/drivers/net/wan/hdlc_cisco.c
> +++ b/drivers/net/wan/hdlc_cisco.c
> @@ -276,8 +276,6 @@ static void cisco_timer(unsigned long arg)
>   spin_unlock(>lock);
>  
>   st->timer.expires = jiffies + st->settings.interval * HZ;
> - st->timer.function = cisco_timer;
> - st->timer.data = arg;
>   add_timer(>timer);
>  }
>  
> diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
> index de42faca076a..7da2424c28a4 100644
> --- a/drivers/net/wan/hdlc_fr.c
> +++ b/drivers/net/wan/hdlc_fr.c
> @@ -644,8 +644,6 @@ static void fr_timer(unsigned long arg)
>   state(hdlc)->settings.t391 * HZ;
>   }
>  
> - state(hdlc)->timer.function = fr_timer;
> - state(hdlc)->timer.data = arg;
>   add_timer((hdlc)->timer);
>  }

-- 
Krzysztof Halasa

[PATCH REPOST] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.

[Krzysztof Halasa]

I think this bug needs to be fixed, this way or another.

The platform in question is Cavium CNS3xxx, ARMv6.

A recent patch by Arnd Bergmann (498a92d42596 "ARM: cns3xxx: pci: avoid
potential stack overflow") converted an explicit setting of
PCI_EXP_DEVCTL_READRQ = 0 (i.e., max 128 bytes for bus-mastering PCIe DMA
read request) to:
+pcie_bus_config = PCIE_BUS_PEER2PEER;

with the following commentary:
"The second part is how the driver sets up the Max_Read_Request_Size
value for the first device/function on bus 1, i.e. the device
plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not
perform the same setting on devices attached to a PCIe switch,
or multi-function devices.
The solution for this part fortunately is even easier: if we
just set the global pcie_bus_config variable to PCIE_BUS_PEER2PEER,
all PCIe devices in the system are limited to 128 byte MPS, which
in turn limits the MRRS to 128 bytes for all devices, and we
no longer even need to touch any devices."

The problem is the MRRS setting is never written to the hardware.
I propose the following, though I'm not sure if we can do this safely,
especially given the comments in probe.c. OTOH, this change may be
required in other (all?) cases when the user requests
PCIE_BUS_PEER2PEER.

On this Laguna GW-2388 the following patch fixes BM DMA with:
:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01)
:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge
:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers)
0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
 the second lane from the CPU)

pci :00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128
pci :01:00.0: Max Payload Size set to  128/ 512 (was  128), Max Read Rq  128
pci 0001:00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128

Signed-off-by: Krzysztof Hałasa 
Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..91713b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)
/* In the "safe" case, do not configure the MRRS.  There appear to be
 * issues with setting MRRS to 0 on a number of devices.
 */
-   if (pcie_bus_config != PCIE_BUS_PERFORMANCE)
+   if (pcie_bus_config != PCIE_BUS_PERFORMANCE &&
+   pcie_bus_config != PCIE_BUS_PEER2PEER)
return;
 
/* For Max performance, the MRRS must be set to the largest supported
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2771625..6f5088a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -756,7 +756,7 @@ enum pcie_bus_config_types {
PCIE_BUS_DEFAULT,   /* ensure MPS matches upstream bridge */
PCIE_BUS_SAFE,  /* use largest MPS boot-time devices support */
PCIE_BUS_PERFORMANCE,   /* use MPS and MRRS for best performance */
-   PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */
+   PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */
 };
 
 extern enum pcie_bus_config_types pcie_bus_config;

[PATCH REPOST] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.

[Krzysztof Halasa]

I think this bug needs to be fixed, this way or another.

The platform in question is Cavium CNS3xxx, ARMv6.

A recent patch by Arnd Bergmann (498a92d42596 "ARM: cns3xxx: pci: avoid
potential stack overflow") converted an explicit setting of
PCI_EXP_DEVCTL_READRQ = 0 (i.e., max 128 bytes for bus-mastering PCIe DMA
read request) to:
+pcie_bus_config = PCIE_BUS_PEER2PEER;

with the following commentary:
"The second part is how the driver sets up the Max_Read_Request_Size
value for the first device/function on bus 1, i.e. the device
plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not
perform the same setting on devices attached to a PCIe switch,
or multi-function devices.
The solution for this part fortunately is even easier: if we
just set the global pcie_bus_config variable to PCIE_BUS_PEER2PEER,
all PCIe devices in the system are limited to 128 byte MPS, which
in turn limits the MRRS to 128 bytes for all devices, and we
no longer even need to touch any devices."

The problem is the MRRS setting is never written to the hardware.
I propose the following, though I'm not sure if we can do this safely,
especially given the comments in probe.c. OTOH, this change may be
required in other (all?) cases when the user requests
PCIE_BUS_PEER2PEER.

On this Laguna GW-2388 the following patch fixes BM DMA with:
:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01)
:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge
:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers)
0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
 the second lane from the CPU)

pci :00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128
pci :01:00.0: Max Payload Size set to  128/ 512 (was  128), Max Read Rq  128
pci 0001:00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128

Signed-off-by: Krzysztof Hałasa 
Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..91713b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)
/* In the "safe" case, do not configure the MRRS.  There appear to be
 * issues with setting MRRS to 0 on a number of devices.
 */
-   if (pcie_bus_config != PCIE_BUS_PERFORMANCE)
+   if (pcie_bus_config != PCIE_BUS_PERFORMANCE &&
+   pcie_bus_config != PCIE_BUS_PEER2PEER)
return;
 
/* For Max performance, the MRRS must be set to the largest supported
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2771625..6f5088a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -756,7 +756,7 @@ enum pcie_bus_config_types {
PCIE_BUS_DEFAULT,   /* ensure MPS matches upstream bridge */
PCIE_BUS_SAFE,  /* use largest MPS boot-time devices support */
PCIE_BUS_PERFORMANCE,   /* use MPS and MRRS for best performance */
-   PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */
+   PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */
 };
 
 extern enum pcie_bus_config_types pcie_bus_config;

[PATCH] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.

[Krzysztof Halasa]

The platform in question is Cavium CNS3xxx, ARMv6.

A recent patch by Arnd Bergmann (498a92d42596 "ARM: cns3xxx: pci: avoid
potential stack overflow") converted an explicit setting of
PCI_EXP_DEVCTL_READRQ = 0 (i.e., max 128 bytes for bus-mastering PCIe DMA
read request) to:
+pcie_bus_config = PCIE_BUS_PEER2PEER;

with the following commentary:
"The second part is how the driver sets up the Max_Read_Request_Size
value for the first device/function on bus 1, i.e. the device
plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not
perform the same setting on devices attached to a PCIe switch,
or multi-function devices.
The solution for this part fortunately is even easier: if we
just set the global pcie_bus_config variable to PCIE_BUS_PEER2PEER,
all PCIe devices in the system are limited to 128 byte MPS, which
in turn limits the MRRS to 128 bytes for all devices, and we
no longer even need to touch any devices."

The problem is the MRRS setting is never written to the hardware.
I propose the following, though I'm not sure if we can do this safely,
especially given the comments in probe.c. OTOH, this change may be
required in other (all?) cases when the user requests
PCIE_BUS_PEER2PEER.

On this Laguna GW-2388 the following patch fixes BM DMA with:
:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01)
:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge
:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers)
0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
 the second lane from the CPU)

pci :00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128
pci :01:00.0: Max Payload Size set to  128/ 512 (was  128), Max Read Rq  128
pci 0001:00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128

Signed-off-by: Krzysztof Hałasa 
Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..91713b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)
/* In the "safe" case, do not configure the MRRS.  There appear to be
 * issues with setting MRRS to 0 on a number of devices.
 */
-   if (pcie_bus_config != PCIE_BUS_PERFORMANCE)
+   if (pcie_bus_config != PCIE_BUS_PERFORMANCE &&
+   pcie_bus_config != PCIE_BUS_PEER2PEER)
return;
 
/* For Max performance, the MRRS must be set to the largest supported
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2771625..6f5088a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -756,7 +756,7 @@ enum pcie_bus_config_types {
PCIE_BUS_DEFAULT,   /* ensure MPS matches upstream bridge */
PCIE_BUS_SAFE,  /* use largest MPS boot-time devices support */
PCIE_BUS_PERFORMANCE,   /* use MPS and MRRS for best performance */
-   PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */
+   PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */
 };
 
 extern enum pcie_bus_config_types pcie_bus_config;

[PATCH] Extend PCIE_BUS_PEER2PEER to set MRSS=128 to fix CNS3xxx BM DMA.

[Krzysztof Halasa]

The platform in question is Cavium CNS3xxx, ARMv6.

A recent patch by Arnd Bergmann (498a92d42596 "ARM: cns3xxx: pci: avoid
potential stack overflow") converted an explicit setting of
PCI_EXP_DEVCTL_READRQ = 0 (i.e., max 128 bytes for bus-mastering PCIe DMA
read request) to:
+pcie_bus_config = PCIE_BUS_PEER2PEER;

with the following commentary:
"The second part is how the driver sets up the Max_Read_Request_Size
value for the first device/function on bus 1, i.e. the device
plugged directly into the PCIe root port.
For all I can tell, this is in fact incomplete, as it does not
perform the same setting on devices attached to a PCIe switch,
or multi-function devices.
The solution for this part fortunately is even easier: if we
just set the global pcie_bus_config variable to PCIE_BUS_PEER2PEER,
all PCIe devices in the system are limited to 128 byte MPS, which
in turn limits the MRRS to 128 bytes for all devices, and we
no longer even need to touch any devices."

The problem is the MRRS setting is never written to the hardware.
I propose the following, though I'm not sure if we can do this safely,
especially given the comments in probe.c. OTOH, this change may be
required in other (all?) cases when the user requests
PCIE_BUS_PEER2PEER.

On this Laguna GW-2388 the following patch fixes BM DMA with:
:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01)
:01:00.0 PCI bridge: Texas Instruments XIO2001 PCI Express-to-PCI Bridge
:02:0e.0 (PCI devices behind the bridge, these are doing actual BM xfers)
0001:00:00.0 PCI bridge: Cavium Networks Device 3400 (rev 01 - this is
 the second lane from the CPU)

pci :00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128
pci :01:00.0: Max Payload Size set to  128/ 512 (was  128), Max Read Rq  128
pci 0001:00:00.0: Max Payload Size set to  128/ 128 (was  128), Max Read Rq  128

Signed-off-by: Krzysztof Hałasa 
Fixes: 498a92d42596 ("ARM: cns3xxx: pci: avoid potential stack overflow")

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6d7ab9b..91713b6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1919,7 +1919,8 @@ static void pcie_write_mrrs(struct pci_dev *dev)
/* In the "safe" case, do not configure the MRRS.  There appear to be
 * issues with setting MRRS to 0 on a number of devices.
 */
-   if (pcie_bus_config != PCIE_BUS_PERFORMANCE)
+   if (pcie_bus_config != PCIE_BUS_PERFORMANCE &&
+   pcie_bus_config != PCIE_BUS_PEER2PEER)
return;
 
/* For Max performance, the MRRS must be set to the largest supported
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2771625..6f5088a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -756,7 +756,7 @@ enum pcie_bus_config_types {
PCIE_BUS_DEFAULT,   /* ensure MPS matches upstream bridge */
PCIE_BUS_SAFE,  /* use largest MPS boot-time devices support */
PCIE_BUS_PERFORMANCE,   /* use MPS and MRRS for best performance */
-   PCIE_BUS_PEER2PEER, /* set MPS = 128 for all devices */
+   PCIE_BUS_PEER2PEER, /* set MPS and MRSS to 128 for all devices */
 };
 
 extern enum pcie_bus_config_types pcie_bus_config;

Re: [PATCH] net: wan: wanxl.c: Cleaning up declaration of a while loop

[Krzysztof Halasa]

Rickard Strandqvist  writes:

> Unusual declaration of a while loop.
> However, believe you also want to make sure that the pointer is not NULL

Not really. The code is meant to do exactly what it currently does -
set variable desc and then check desc->stat.
All rx_descs are at this point already initialized and not NULL
(if desc was indeed NULL we better BUG*() or Oops on desc->stat access
instead of failing silently).

> --- a/drivers/net/wan/wanxl.c
> +++ b/drivers/net/wan/wanxl.c
> @@ -196,7 +196,7 @@ static inline void wanxl_tx_intr(port_t *port)
>  static inline void wanxl_rx_intr(card_t *card)
>  {
>   desc_t *desc;
> - while (desc = >status->rx_descs[card->rx_in],
> + while (desc = >status->rx_descs[card->rx_in] &&
>  desc->stat != PACKET_EMPTY) {
>   if ((desc->stat & PACKET_PORT_MASK) > card->n_ports)
>   pr_crit("%s: received packet for nonexistent port\n",

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

Re: [PATCH] net: wan: wanxl.c: Cleaning up declaration of a while loop

[Krzysztof Halasa]

Rickard Strandqvist rickard_strandqv...@spectrumdigital.se writes:

 Unusual declaration of a while loop.
 However, believe you also want to make sure that the pointer is not NULL

Not really. The code is meant to do exactly what it currently does -
set variable desc and then check desc-stat.
All rx_descs are at this point already initialized and not NULL
(if desc was indeed NULL we better BUG*() or Oops on desc-stat access
instead of failing silently).

 --- a/drivers/net/wan/wanxl.c
 +++ b/drivers/net/wan/wanxl.c
 @@ -196,7 +196,7 @@ static inline void wanxl_tx_intr(port_t *port)
  static inline void wanxl_rx_intr(card_t *card)
  {
   desc_t *desc;
 - while (desc = card-status-rx_descs[card-rx_in],
 + while (desc = card-status-rx_descs[card-rx_in] 
  desc-stat != PACKET_EMPTY) {
   if ((desc-stat  PACKET_PORT_MASK)  card-n_ports)
   pr_crit(%s: received packet for nonexistent port\n,

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

Re: [PATCH 06/18] ARM: IXP4xx: Switch to sched_clock_register()

[Krzysztof Halasa]

Stephen Boyd  writes:

> The 32 bit sched_clock interface now supports 64 bits. Upgrade to
> the 64 bit function to allow us to remove the 32 bit registration
> interface.

Don't find anything wrong in this.

Acked-by: Krzysztof Halasa 

> +++ b/arch/arm/mach-ixp4xx/common.c
> @@ -448,7 +448,7 @@ void __init ixp4xx_sys_init(void)
>  /*
>   * sched_clock()
>   */
> -static u32 notrace ixp4xx_read_sched_clock(void)
> +static u64 notrace ixp4xx_read_sched_clock(void)
>  {
>   return *IXP4XX_OSTS;
>  }
> @@ -466,7 +466,7 @@ unsigned long ixp4xx_timer_freq = IXP4XX_TIMER_FREQ;
>  EXPORT_SYMBOL(ixp4xx_timer_freq);
>  static void __init ixp4xx_clocksource_init(void)
>  {
> - setup_sched_clock(ixp4xx_read_sched_clock, 32, ixp4xx_timer_freq);
> + sched_clock_register(ixp4xx_read_sched_clock, 32, ixp4xx_timer_freq);
>  
>   clocksource_mmio_init(NULL, "OSTS", ixp4xx_timer_freq, 200, 32,
>   ixp4xx_clocksource_read);

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

Re: [PATCH 06/18] ARM: IXP4xx: Switch to sched_clock_register()

[Krzysztof Halasa]

Stephen Boyd sb...@codeaurora.org writes:

 The 32 bit sched_clock interface now supports 64 bits. Upgrade to
 the 64 bit function to allow us to remove the 32 bit registration
 interface.

Don't find anything wrong in this.

Acked-by: Krzysztof Halasa k...@pm.waw.pl

 +++ b/arch/arm/mach-ixp4xx/common.c
 @@ -448,7 +448,7 @@ void __init ixp4xx_sys_init(void)
  /*
   * sched_clock()
   */
 -static u32 notrace ixp4xx_read_sched_clock(void)
 +static u64 notrace ixp4xx_read_sched_clock(void)
  {
   return *IXP4XX_OSTS;
  }
 @@ -466,7 +466,7 @@ unsigned long ixp4xx_timer_freq = IXP4XX_TIMER_FREQ;
  EXPORT_SYMBOL(ixp4xx_timer_freq);
  static void __init ixp4xx_clocksource_init(void)
  {
 - setup_sched_clock(ixp4xx_read_sched_clock, 32, ixp4xx_timer_freq);
 + sched_clock_register(ixp4xx_read_sched_clock, 32, ixp4xx_timer_freq);
  
   clocksource_mmio_init(NULL, OSTS, ixp4xx_timer_freq, 200, 32,
   ixp4xx_clocksource_read);

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

Re: [PATCH 1/2] arm: ixp4xx: set cohorent_dma_mask for ethernet platform devices

[Krzysztof Halasa]

Ben Hutchings  writes:

> I think the problem is that the platform device and the platform data
> have not been properly separated.  The machine-specific setup functions
> should be passing the eth_plat_info and MAC ID into a common ixp4xx
> function which would then create the platform device with the correct
> DMA mask etc.

That's also my idea. Will look at this.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] arm: ixp4xx: set cohorent_dma_mask for ethernet platform devices

[Krzysztof Halasa]

Ben Hutchings bhutchi...@solarflare.com writes:

 I think the problem is that the platform device and the platform data
 have not been properly separated.  The machine-specific setup functions
 should be passing the eth_plat_info and MAC ID into a common ixp4xx
 function which would then create the platform device with the correct
 DMA mask etc.

That's also my idea. Will look at this.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] arm: ixp4xx: set cohorent_dma_mask for ethernet platform devices

[Krzysztof Halasa]

Jonas Gorski  writes:

> ARM requires the cohorent_dma_mask set, so set it for the platform
> devices so that the ethernet driver has access to it.

I recognize the need to fix this issue and I appreciate your efforts,
but... I think this patch tries to make the driver functional again at
all costs and this a very bad idea. The IXP4xx Ethernet MACs are not
normal platform devices, they are in fact built-in CPU resources. The
platform device structs are only used to set parameters. What the patch
does is unneeded and IMHO harmful code duplication. It makes completely
no sense to set DMA masks in code for individual platforms as it's not
something platforms can decide, or *even should know of*. It's simply
a CPU attribute, a value that is shared by all IXP4xx CPUs and thus all
platforms and systems using it.

This is against the "line of code" count rules (or "rules").

Also the dev->dev.parent is IMHO a bad idea. The queue numbers and MAC
addresses are in no way "parents" of Ethernet controllers, and even if
they somehow were, I find it rather hard to believe they can have DMA
masks.

I think the previous patch (which sets the masks in one place, in
Ethernet driver code) was better, though not perfect.

My fault is I haven't fixed it yet. Will try to invent something.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] arm: ixp4xx: set cohorent_dma_mask for ethernet platform devices

[Krzysztof Halasa]

Jonas Gorski j...@openwrt.org writes:

 ARM requires the cohorent_dma_mask set, so set it for the platform
 devices so that the ethernet driver has access to it.

I recognize the need to fix this issue and I appreciate your efforts,
but... I think this patch tries to make the driver functional again at
all costs and this a very bad idea. The IXP4xx Ethernet MACs are not
normal platform devices, they are in fact built-in CPU resources. The
platform device structs are only used to set parameters. What the patch
does is unneeded and IMHO harmful code duplication. It makes completely
no sense to set DMA masks in code for individual platforms as it's not
something platforms can decide, or *even should know of*. It's simply
a CPU attribute, a value that is shared by all IXP4xx CPUs and thus all
platforms and systems using it.

This is against the line of code count rules (or rules).

Also the dev-dev.parent is IMHO a bad idea. The queue numbers and MAC
addresses are in no way parents of Ethernet controllers, and even if
they somehow were, I find it rather hard to believe they can have DMA
masks.

I think the previous patch (which sets the masks in one place, in
Ethernet driver code) was better, though not perfect.

My fault is I haven't fixed it yet. Will try to invent something.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 7/8] ARM: ixp4xx: avoid circular header dependency

[Krzysztof Halasa]

Arnd Bergmann  writes:

> With the new linux/reboot.h header file dependency added, we can no
> longer build ixp4xx. The easiest way to avoid that is to remove the
> inclusion of mach/hardware.h from mach/timex.h, which does not need
> that header anyway.

>  arch/arm/mach-ixp4xx/include/mach/timex.h | 2 +-

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

Re: [PATCH 7/8] ARM: ixp4xx: avoid circular header dependency

[Krzysztof Halasa]

Arnd Bergmann a...@arndb.de writes:

 With the new linux/reboot.h header file dependency added, we can no
 longer build ixp4xx. The easiest way to avoid that is to remove the
 inclusion of mach/hardware.h from mach/timex.h, which does not need
 that header anyway.

  arch/arm/mach-ixp4xx/include/mach/timex.h | 2 +-

Acked-by: Krzysztof Halasa k...@pm.waw.pl
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

Russell King - ARM Linux  writes:

> Right, so, the answer is - yes you are talking about platform devices,
> and the reason that these aren't already set is because if you grep for
> ixp4xx_eth or ixp4xx_hss in arch/arm/mach-ixp4xx, you'll notice that
> _none_ of the device declarations set either of the DMA masks at all.
> They don't even set the dev->dma_mask pointer.  That is why the masks
> are zero.  For a device which does DMA, that is wrong.

Well, that's new to me. Please tell me how it should be done.
Should I simply add in platform code (on all platforms):

+++ b/arch/arm/mach-ixp4xx/XXX.c
@@ -380,10 +380,12 @@ static struct platform_device device_eth_tab[] = {
 .name   = "ixp4xx_eth",
 .id = IXP4XX_ETH_NPEB,
 .dev.platform_data  = eth_plat,
+.dev.coherent_dma_mask  = DMA_BIT_MASK(32),
 }, {
 .name   = "ixp4xx_eth",
 .id = IXP4XX_ETH_NPEC,
 .dev.platform_data  = eth_plat + 1,
+.dev.coherent_dma_mask  = DMA_BIT_MASK(32),
 }
 };

This alone isn't going to work, the problem is coherent DMA mask in
net_dev->dev and not in platform_device. So what do I do in the network
drivers? Copy the masks from platform_device to the actual newly created
net_dev->dev?

Should I use the parent device (net_dev->dev.parent which is the
platform_device) as the argument to the allocator? This would probably
work though I'm not sure of its correctness.

BTW the platform code shouldn't IMHO need to declare the masks as they
aren't platform-specific. They are driver-specific (defined by CPU
design) and setting them in the driver seems clean to me (unlike the
platform code).

Several other device drivers simply set their masks directly. Is it the
recommended way?

> If you look at the PCI code, it pre-initializes the DMA mask to be 4GiB:
> drivers/pci/probe.c:dev->dev.coherent_dma_mask = 0xull;
>
> And that is what is missing from the IXP4xx platform code.
>
> However, avoiding the call to dma_set_coherent_mask() from within the
> driver also seems to be questionable as it bypasses the "check if the
> mask is possible" part of the DMA API.

I thought about this for a second but the situation is the mask
is guaranteed to be valid (these are on-chip devices).
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

Russell King - ARM Linux li...@arm.linux.org.uk writes:

 Right, so, the answer is - yes you are talking about platform devices,
 and the reason that these aren't already set is because if you grep for
 ixp4xx_eth or ixp4xx_hss in arch/arm/mach-ixp4xx, you'll notice that
 _none_ of the device declarations set either of the DMA masks at all.
 They don't even set the dev-dma_mask pointer.  That is why the masks
 are zero.  For a device which does DMA, that is wrong.

Well, that's new to me. Please tell me how it should be done.
Should I simply add in platform code (on all platforms):

+++ b/arch/arm/mach-ixp4xx/XXX.c
@@ -380,10 +380,12 @@ static struct platform_device device_eth_tab[] = {
 .name   = ixp4xx_eth,
 .id = IXP4XX_ETH_NPEB,
 .dev.platform_data  = eth_plat,
+.dev.coherent_dma_mask  = DMA_BIT_MASK(32),
 }, {
 .name   = ixp4xx_eth,
 .id = IXP4XX_ETH_NPEC,
 .dev.platform_data  = eth_plat + 1,
+.dev.coherent_dma_mask  = DMA_BIT_MASK(32),
 }
 };

This alone isn't going to work, the problem is coherent DMA mask in
net_dev-dev and not in platform_device. So what do I do in the network
drivers? Copy the masks from platform_device to the actual newly created
net_dev-dev?

Should I use the parent device (net_dev-dev.parent which is the
platform_device) as the argument to the allocator? This would probably
work though I'm not sure of its correctness.

BTW the platform code shouldn't IMHO need to declare the masks as they
aren't platform-specific. They are driver-specific (defined by CPU
design) and setting them in the driver seems clean to me (unlike the
platform code).

Several other device drivers simply set their masks directly. Is it the
recommended way?

 If you look at the PCI code, it pre-initializes the DMA mask to be 4GiB:
 drivers/pci/probe.c:dev-dev.coherent_dma_mask = 0xull;

 And that is what is missing from the IXP4xx platform code.

 However, avoiding the call to dma_set_coherent_mask() from within the
 driver also seems to be questionable as it bypasses the check if the
 mask is possible part of the DMA API.

I thought about this for a second but the situation is the mask
is guaranteed to be valid (these are on-chip devices).
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

Russell King - ARM Linux  writes:

> I'm having a hard time understanding what is an ARM issue here, what is
> an ARM bug, and what the DMA API requires.  The DMA API documentation
> is extremely sparse in describing what's required of the DMA masks,
> what these functions are supposed to do, and what determines whether
> a mask is "possible" or not.
>
> Moreover, I'm also having a hard time understanding what broke in 3.7,
> and why this fixes it.
>
> In other words, I'm completely failing to understand everything about
> this patch.

The ARM issue here is that the coherent DMA mask, by default, is zero
(i.e. coherent allocations are impossible by default UNLESS the device
pointer supplied is NULL - since DMA masks are bound to devices).
On most other platforms, the default DMA mask is 0x = (u32)-1
and this is also required by the DMA API.

In v3.7 (between v3.7-rc6 and v3.7-rc7), Xi Wang noticed that IXP4xx net
drivers call dma_pool_create() with NULL dev argument, and changed them
to use >netdev->dev. This broke coherent allocations since now the
device supplied to dma_pool_create() is not NULL and the (zero) mask
prevents coherent allocations. This means the Ethernet and HSS drivers
are since then unusable.


The first part of my patch makes dma_set_coherent_mask (IXP4xx-only
code) actually set the mask. This is needed as on IXP4xx we have (at
least) two different situations:

- PCI devices want "DMA zone" memory (26 bits = 64 MiB)
- built-in devices can use any RAM (32 bits = 4 GiB).

Without the patch, dma_set_coherent_mask only returns 0 or -EIO, it
doesn't change the actual coherent DMA mask and it's then impossible for
coherent allocators to differentiate between the above two cases.

+++ b/arch/arm/mach-ixp4xx/common-pci.c
@@ -454,10 +454,15 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)
 
 int dma_set_coherent_mask(struct device *dev, u64 mask)
 {
-   if (mask >= SZ_64M - 1)
-   return 0;
+   if ((mask & DMA_BIT_MASK(26)) != DMA_BIT_MASK(26))
+   return -EIO;
+
+   /* PCI devices are limited to 64 MiB */
+   if (dev_is_pci(dev))
+   mask &= DMA_BIT_MASK(26); /* Use DMA region */
 
-   return -EIO;
+   dev->coherent_dma_mask = mask;
+   return 0;
 }


The second part of my patch sets the coherent DMA masks of the Ethernet
and HSS devices to 4 GiB (the value which should already be the
default). This also seems to be a recommended action.

+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -1423,7 +1423,7 @@ static int eth_init_one(struct platform_device *pdev)
dev->ethtool_ops = _ethtool_ops;
dev->tx_queue_len = 100;
-
+   dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
netif_napi_add(dev, >napi, eth_poll, NAPI_WEIGHT);
 
+++ b/drivers/net/wan/ixp4xx_hss.c
@@ -1367,6 +1367,7 @@ static int hss_init_one(struct platform_device *pdev)
port->dev = >dev;
port->plat = pdev->dev.platform_data;
+   dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
netif_napi_add(dev, >napi, hss_hdlc_poll, NAPI_WEIGHT);

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

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

David Miller  writes:

>> ARM core code currently requires coherent DMA mask to be set.

> This requirement is not reasonable.
>
> The DMA API documentation clearly states what the default must be,
> and what drivers are guarenteed will be the default.

ARM people - what do you think?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

David Miller da...@davemloft.net writes:

 ARM core code currently requires coherent DMA mask to be set.

 This requirement is not reasonable.

 The DMA API documentation clearly states what the default must be,
 and what drivers are guarenteed will be the default.

ARM people - what do you think?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

Russell King - ARM Linux li...@arm.linux.org.uk writes:

 I'm having a hard time understanding what is an ARM issue here, what is
 an ARM bug, and what the DMA API requires.  The DMA API documentation
 is extremely sparse in describing what's required of the DMA masks,
 what these functions are supposed to do, and what determines whether
 a mask is possible or not.

 Moreover, I'm also having a hard time understanding what broke in 3.7,
 and why this fixes it.

 In other words, I'm completely failing to understand everything about
 this patch.

The ARM issue here is that the coherent DMA mask, by default, is zero
(i.e. coherent allocations are impossible by default UNLESS the device
pointer supplied is NULL - since DMA masks are bound to devices).
On most other platforms, the default DMA mask is 0x = (u32)-1
and this is also required by the DMA API.

In v3.7 (between v3.7-rc6 and v3.7-rc7), Xi Wang noticed that IXP4xx net
drivers call dma_pool_create() with NULL dev argument, and changed them
to use port-netdev-dev. This broke coherent allocations since now the
device supplied to dma_pool_create() is not NULL and the (zero) mask
prevents coherent allocations. This means the Ethernet and HSS drivers
are since then unusable.


The first part of my patch makes dma_set_coherent_mask (IXP4xx-only
code) actually set the mask. This is needed as on IXP4xx we have (at
least) two different situations:

- PCI devices want DMA zone memory (26 bits = 64 MiB)
- built-in devices can use any RAM (32 bits = 4 GiB).

Without the patch, dma_set_coherent_mask only returns 0 or -EIO, it
doesn't change the actual coherent DMA mask and it's then impossible for
coherent allocators to differentiate between the above two cases.

+++ b/arch/arm/mach-ixp4xx/common-pci.c
@@ -454,10 +454,15 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)
 
 int dma_set_coherent_mask(struct device *dev, u64 mask)
 {
-   if (mask = SZ_64M - 1)
-   return 0;
+   if ((mask  DMA_BIT_MASK(26)) != DMA_BIT_MASK(26))
+   return -EIO;
+
+   /* PCI devices are limited to 64 MiB */
+   if (dev_is_pci(dev))
+   mask = DMA_BIT_MASK(26); /* Use DMA region */
 
-   return -EIO;
+   dev-coherent_dma_mask = mask;
+   return 0;
 }


The second part of my patch sets the coherent DMA masks of the Ethernet
and HSS devices to 4 GiB (the value which should already be the
default). This also seems to be a recommended action.

+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -1423,7 +1423,7 @@ static int eth_init_one(struct platform_device *pdev)
dev-ethtool_ops = ixp4xx_ethtool_ops;
dev-tx_queue_len = 100;
-
+   dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32));
netif_napi_add(dev, port-napi, eth_poll, NAPI_WEIGHT);
 
+++ b/drivers/net/wan/ixp4xx_hss.c
@@ -1367,6 +1367,7 @@ static int hss_init_one(struct platform_device *pdev)
port-dev = pdev-dev;
port-plat = pdev-dev.platform_data;
+   dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32));
netif_napi_add(dev, port-napi, hss_hdlc_poll, NAPI_WEIGHT);

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

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

Ben Hutchings  writes:

> I'm failing to see where it says the default can't be narrower than 32
> bits due to platform limits.  And how do you think DMA mapping is
> supposed to work for PCI devices on these platforms, anyway?

The problem on ARM (and probably on powerpc, and on something called
"metag" - grep -r 'coherent DMA mask is unset' arch) is that the default
coherent DMA mask is zero. IOW, coherent DMA allocations are, by
default, disabled. A driver has to dma_set_coherent_mask() or, as many
drivers do, set dev->coherent_dma_mask directly (IMHO
dev->coherent_dma_mask along with dev->dma_mask are private DMA API
stuff and e.g. device drivers have no interest there).

The zero default is IMHO, WRT the actual DMA API, an ARM bug (and
powerpc's etc). Nevertheless, the patch I posted does everything as
required by the API. Specifically, the IXP4xx arch part makes
IXP4xx's dma_set_coherent_mask() compliant with DMA API, and the actual
dma_set_coherent_mask() calls in drivers are both valid and I guess
recommended by the API.

The patch doesn't touch the core ARM issue, that's right.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

Ben Hutchings bhutchi...@solarflare.com writes:

 I'm failing to see where it says the default can't be narrower than 32
 bits due to platform limits.  And how do you think DMA mapping is
 supposed to work for PCI devices on these platforms, anyway?

The problem on ARM (and probably on powerpc, and on something called
metag - grep -r 'coherent DMA mask is unset' arch) is that the default
coherent DMA mask is zero. IOW, coherent DMA allocations are, by
default, disabled. A driver has to dma_set_coherent_mask() or, as many
drivers do, set dev-coherent_dma_mask directly (IMHO
dev-coherent_dma_mask along with dev-dma_mask are private DMA API
stuff and e.g. device drivers have no interest there).

The zero default is IMHO, WRT the actual DMA API, an ARM bug (and
powerpc's etc). Nevertheless, the patch I posted does everything as
required by the API. Specifically, the IXP4xx arch part makes
IXP4xx's dma_set_coherent_mask() compliant with DMA API, and the actual
dma_set_coherent_mask() calls in drivers are both valid and I guess
recommended by the API.

The patch doesn't touch the core ARM issue, that's right.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

ARM core code currently requires coherent DMA mask to be set. Make sure
we limit PCI devices to 64 MiB while allowing on-chip devices to access
the whole 4 GiB address space.

This fixes a v3.7+ regression which broke IXP4xx built-in network devices.

Signed-off-by: Krzysztof Hałasa 
Cc: 

diff --git a/arch/arm/mach-ixp4xx/common-pci.c 
b/arch/arm/mach-ixp4xx/common-pci.c
index ea1933d..8629fc9 100644
--- a/arch/arm/mach-ixp4xx/common-pci.c
+++ b/arch/arm/mach-ixp4xx/common-pci.c
@@ -454,10 +454,15 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)
 
 int dma_set_coherent_mask(struct device *dev, u64 mask)
 {
-   if (mask >= SZ_64M - 1)
-   return 0;
+   if ((mask & DMA_BIT_MASK(26)) != DMA_BIT_MASK(26))
+   return -EIO;
+
+   /* PCI devices are limited to 64 MiB */
+   if (dev_is_pci(dev))
+   mask &= DMA_BIT_MASK(26); /* Use DMA region */
 
-   return -EIO;
+   dev->coherent_dma_mask = mask;
+   return 0;
 }
 
 EXPORT_SYMBOL(ixp4xx_pci_read);
diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c 
b/drivers/net/ethernet/xscale/ixp4xx_eth.c
index 6958a5e..7c08269 100644
--- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -1423,7 +1423,7 @@ static int eth_init_one(struct platform_device *pdev)
dev->netdev_ops = _netdev_ops;
dev->ethtool_ops = _ethtool_ops;
dev->tx_queue_len = 100;
-
+   dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
netif_napi_add(dev, >napi, eth_poll, NAPI_WEIGHT);
 
if (!(port->npe = npe_request(NPE_ID(port->id {
diff --git a/drivers/net/wan/ixp4xx_hss.c b/drivers/net/wan/ixp4xx_hss.c
index 95d0451..83b4597 100644
--- a/drivers/net/wan/ixp4xx_hss.c
+++ b/drivers/net/wan/ixp4xx_hss.c
@@ -1367,6 +1367,7 @@ static int hss_init_one(struct platform_device *pdev)
port->id = pdev->id;
port->dev = >dev;
port->plat = pdev->dev.platform_data;
+   dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
netif_napi_add(dev, >napi, hss_hdlc_poll, NAPI_WEIGHT);
 
if ((err = register_hdlc_device(dev)))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] Fix IXP4xx coherent allocations

[Krzysztof Halasa]

ARM core code currently requires coherent DMA mask to be set. Make sure
we limit PCI devices to 64 MiB while allowing on-chip devices to access
the whole 4 GiB address space.

This fixes a v3.7+ regression which broke IXP4xx built-in network devices.

Signed-off-by: Krzysztof Hałasa k...@pm.waw.pl
Cc: sta...@vger.kernel.org

diff --git a/arch/arm/mach-ixp4xx/common-pci.c 
b/arch/arm/mach-ixp4xx/common-pci.c
index ea1933d..8629fc9 100644
--- a/arch/arm/mach-ixp4xx/common-pci.c
+++ b/arch/arm/mach-ixp4xx/common-pci.c
@@ -454,10 +454,15 @@ int ixp4xx_setup(int nr, struct pci_sys_data *sys)
 
 int dma_set_coherent_mask(struct device *dev, u64 mask)
 {
-   if (mask = SZ_64M - 1)
-   return 0;
+   if ((mask  DMA_BIT_MASK(26)) != DMA_BIT_MASK(26))
+   return -EIO;
+
+   /* PCI devices are limited to 64 MiB */
+   if (dev_is_pci(dev))
+   mask = DMA_BIT_MASK(26); /* Use DMA region */
 
-   return -EIO;
+   dev-coherent_dma_mask = mask;
+   return 0;
 }
 
 EXPORT_SYMBOL(ixp4xx_pci_read);
diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c 
b/drivers/net/ethernet/xscale/ixp4xx_eth.c
index 6958a5e..7c08269 100644
--- a/drivers/net/ethernet/xscale/ixp4xx_eth.c
+++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
@@ -1423,7 +1423,7 @@ static int eth_init_one(struct platform_device *pdev)
dev-netdev_ops = ixp4xx_netdev_ops;
dev-ethtool_ops = ixp4xx_ethtool_ops;
dev-tx_queue_len = 100;
-
+   dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32));
netif_napi_add(dev, port-napi, eth_poll, NAPI_WEIGHT);
 
if (!(port-npe = npe_request(NPE_ID(port-id {
diff --git a/drivers/net/wan/ixp4xx_hss.c b/drivers/net/wan/ixp4xx_hss.c
index 95d0451..83b4597 100644
--- a/drivers/net/wan/ixp4xx_hss.c
+++ b/drivers/net/wan/ixp4xx_hss.c
@@ -1367,6 +1367,7 @@ static int hss_init_one(struct platform_device *pdev)
port-id = pdev-id;
port-dev = pdev-dev;
port-plat = pdev-dev.platform_data;
+   dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32));
netif_napi_add(dev, port-napi, hss_hdlc_poll, NAPI_WEIGHT);
 
if ((err = register_hdlc_device(dev)))
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ixp4xx_eth: set the device dma_coherent_mask

[Krzysztof Halasa]

Christophe Aeschlimann  writes:

> Without the mask it is impossible to take the network interface up
> since it returns the following error:
>
>> net eth1: coherent DMA mask is unset
>> ifconfig: SIOCSIFFLAGS: Cannot allocate memory
>
> +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
> @@ -1398,6 +1398,7 @@ static int eth_init_one(struct platform_device *pdev)
>   return -ENOMEM;
>  
>   SET_NETDEV_DEV(dev, >dev);
> + dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>   port = netdev_priv(dev);
>   port->netdev = dev;
>   port->id = pdev->id;

This happens to work but seems to me like a bad "solution". The real
issue is that current ARM coherent_dma_mask (and regular dma_mask)
handling is just broken (at least on platforms with
CONFIG_ARCH_HAS_DMA_SET_COHERENT_MASK). I'm thinking how can it be
fixed.

The problematic parts include the dmabouncer (which takes over DMA
operations and provide services similar to swiotlb), the host DMA and
coherent DMA masks (which are property of the buses) and the device
masks, which should be ANDed with bus masks (but aren't).

What the driver should use is:
err = dma_set_coherent_mask(xxx);

(but the subsystem have to be fixed first).
Also, according to DMA-API, 32-bit is the default.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ixp4xx_eth: set the device dma_coherent_mask

[Krzysztof Halasa]

Christophe Aeschlimann aeschlim...@gmail.com writes:

 Without the mask it is impossible to take the network interface up
 since it returns the following error:

 net eth1: coherent DMA mask is unset
 ifconfig: SIOCSIFFLAGS: Cannot allocate memory

 +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c
 @@ -1398,6 +1398,7 @@ static int eth_init_one(struct platform_device *pdev)
   return -ENOMEM;
  
   SET_NETDEV_DEV(dev, pdev-dev);
 + dev-dev.coherent_dma_mask = DMA_BIT_MASK(32);
   port = netdev_priv(dev);
   port-netdev = dev;
   port-id = pdev-id;

This happens to work but seems to me like a bad solution. The real
issue is that current ARM coherent_dma_mask (and regular dma_mask)
handling is just broken (at least on platforms with
CONFIG_ARCH_HAS_DMA_SET_COHERENT_MASK). I'm thinking how can it be
fixed.

The problematic parts include the dmabouncer (which takes over DMA
operations and provide services similar to swiotlb), the host DMA and
coherent DMA masks (which are property of the buses) and the device
masks, which should be ANDed with bus masks (but aren't).

What the driver should use is:
err = dma_set_coherent_mask(xxx);

(but the subsystem have to be fixed first).
Also, according to DMA-API, 32-bit is the default.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8] wan: Remove unnecessary alloc/OOM messages

[Krzysztof Halasa]

Joe Perches  writes:

> alloc failures already get standardized OOM
> messages and a dump_stack.
>
> Hoist assigns from if tests.
>
> Signed-off-by: Joe Perches 
> ---
>  drivers/net/wan/cosa.c| 9 -
>  drivers/net/wan/farsync.c | 6 ++
>  drivers/net/wan/hdlc.c| 9 -
>  drivers/net/wan/x25_asy.c | 1 -
>  4 files changed, 10 insertions(+), 15 deletions(-)

Good to see.

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

Re: [PATCH 5/8] wan: Remove unnecessary alloc/OOM messages

[Krzysztof Halasa]

Joe Perches j...@perches.com writes:

 alloc failures already get standardized OOM
 messages and a dump_stack.

 Hoist assigns from if tests.

 Signed-off-by: Joe Perches j...@perches.com
 ---
  drivers/net/wan/cosa.c| 9 -
  drivers/net/wan/farsync.c | 6 ++
  drivers/net/wan/hdlc.c| 9 -
  drivers/net/wan/x25_asy.c | 1 -
  4 files changed, 10 insertions(+), 15 deletions(-)

Good to see.

Acked-by: Krzysztof Halasa k...@pm.waw.pl
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PULL REQ] IXP4xx changes for Linux 3.7

[Krzysztof Halasa]

Hi,

Arnd Bergmann  writes:

> as mentioned before, all arch/arm/mach-* patches should go through the
> arm-soc tree or get an Ack from the arm-soc maintainers. The same thing
> is true for the char-misc and the crypto trees.

You've seen the changes. They were trivial fixes, touched only
IXP4xx-related areas, there was nothing related to char or crypto
subsystems there, except file location (well, IXP4xx drivers have to be
located somewhere).

The patches have been posted to linux-arm-kernel and you knew about
them. That they were obviously correct and clean is probably not
that important.

Nobody bothered to comment the patches.

> Also, never rebase your tree immediately before sending a pull
> request.

I did not, of course. My mail stated:
"Build-tested for now. This is based on your current tree tip because it
depends on commits following 3.6 release."

Normally I wouldn't rebase, but had to (as you well knew) - because you
commited a conflicting patch to this very IXP4xx arch. Using your logic,
you were supposed to get an Ack from me (or from Imre) for this patch.

Actually it wasn't a problem for me, I simply had to rebase.

> Finally when sending bug fixes, please annotate any patches with
> 'Cc: sta...@vger.kernel.org' if they address bugs that are already
> present in older kernels, so that the stable and longterm maintainers
> can easily backport the fixes.

I do that when I think it makes sense. In this case (two patches for
Goramo MultiLink platform) practically all such devices use kernels
prepared by me, and I think Greg (and others) have more efficient ways
to spend their time than handling almost unused patches. Also, I have
much more efficient ways to spend time. Anyway, if I can't have my
patches upstream, why should they end up in stable?

> Almost all of the platform patches in your tree seem to be bug fixes,
> so they are still good for inclusion in v3.7 if you submit them to
> arm-soc soon, but please make sure you separate bug fixes from other
> changes so we can group them appropriately when forwarding them to
> Linus.

Unfortunately, as I already explained to you in
https://lkml.org/lkml/2012/9/29/37, my resources for IXP4xx are very
limited (and this isn't a paid job) and I'm in no way able to do what
you require. This, coupled with my inability to make the patches end up
upstream any other way, will make me post relevant MAINTAINERS changes
shortly.

Don't get me wrong. If I had time for this it could be different.
Unfortunately IXP4xx is a legacy arch, and for me it's simply a hobby at
this point. Given the raised barriers to participate, probably aimed at
paid maintainers, I have to quit doing this.

BTW since Imre has probably even much less time, it would be a good time
to find someone to maintain IXP4xx code. I will be publishing (from time
to time) my tree (I'm using the hw myself), so even simple
cherry-picking would probably make some sense.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PULL REQ] IXP4xx changes for Linux 3.7

[Krzysztof Halasa]

Hi,

Arnd Bergmann a...@arndb.de writes:

 as mentioned before, all arch/arm/mach-* patches should go through the
 arm-soc tree or get an Ack from the arm-soc maintainers. The same thing
 is true for the char-misc and the crypto trees.

You've seen the changes. They were trivial fixes, touched only
IXP4xx-related areas, there was nothing related to char or crypto
subsystems there, except file location (well, IXP4xx drivers have to be
located somewhere).

The patches have been posted to linux-arm-kernel and you knew about
them. That they were obviously correct and clean is probably not
that important.

Nobody bothered to comment the patches.

 Also, never rebase your tree immediately before sending a pull
 request.

I did not, of course. My mail stated:
Build-tested for now. This is based on your current tree tip because it
depends on commits following 3.6 release.

Normally I wouldn't rebase, but had to (as you well knew) - because you
commited a conflicting patch to this very IXP4xx arch. Using your logic,
you were supposed to get an Ack from me (or from Imre) for this patch.

Actually it wasn't a problem for me, I simply had to rebase.

 Finally when sending bug fixes, please annotate any patches with
 'Cc: sta...@vger.kernel.org' if they address bugs that are already
 present in older kernels, so that the stable and longterm maintainers
 can easily backport the fixes.

I do that when I think it makes sense. In this case (two patches for
Goramo MultiLink platform) practically all such devices use kernels
prepared by me, and I think Greg (and others) have more efficient ways
to spend their time than handling almost unused patches. Also, I have
much more efficient ways to spend time. Anyway, if I can't have my
patches upstream, why should they end up in stable?

 Almost all of the platform patches in your tree seem to be bug fixes,
 so they are still good for inclusion in v3.7 if you submit them to
 arm-soc soon, but please make sure you separate bug fixes from other
 changes so we can group them appropriately when forwarding them to
 Linus.

Unfortunately, as I already explained to you in
https://lkml.org/lkml/2012/9/29/37, my resources for IXP4xx are very
limited (and this isn't a paid job) and I'm in no way able to do what
you require. This, coupled with my inability to make the patches end up
upstream any other way, will make me post relevant MAINTAINERS changes
shortly.

Don't get me wrong. If I had time for this it could be different.
Unfortunately IXP4xx is a legacy arch, and for me it's simply a hobby at
this point. Given the raised barriers to participate, probably aimed at
paid maintainers, I have to quit doing this.

BTW since Imre has probably even much less time, it would be a good time
to find someone to maintain IXP4xx code. I will be publishing (from time
to time) my tree (I'm using the hw myself), so even simple
cherry-picking would probably make some sense.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PULL REQ] IXP4xx changes for Linux 3.7

[Krzysztof Halasa]

Linus,

please pull my ARM IXP4xx changes for 3.7:

The following changes since commit 4d7127dace8cf4b05eb7c8c8531fc204fbb195f4:

"Merge branch 'for-linus' of git://git.kernel.org/.../jmorris/linux-security"
(2012-10-13 11:29:00 +0900)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/chris/linux.git next

for you to fetch changes up to b94740b3b38fd8e37fcd3bb06a18ec2796061c7d:
  IXP4xx: use __iomem for MMIO (2012-10-13 20:37:30 +0200)

Build-tested for now. This is based on your current tree tip because it
depends on commits following 3.6 release.

Thanks.


Arnd Bergmann (1):
  IXP4xx: use __iomem for MMIO

Krzysztof Hałasa (9):
  IXP4xx: Fix Goramo MultiLink platform compilation.
  IXP4xx: Fix off-by-one bug in Goramo MultiLink platform.
  IXP4xx: HW pseudo-random generator is available on IXP45x/46x only.
  IXP4xx: ixp4xx_crypto driver requires Queue Manager and NPE drivers.
  IXP4xx: Remove time limit for PCI TRDY to enable use of slow devices.
  WAN: Remove redundant HDLC info printed by IXP4xx HSS driver.
  IXP4xx crypto: MOD_AES{128,192,256} already include key size.
  IXP4xx: Always ioremap() Queue Manager MMIO region at boot.
  IXP4xx: map CPU config registers within VMALLOC region.

Tim Gardner (1):
  ixp4xx: Declare MODULE_FIRMWARE usage

 arch/arm/mach-ixp4xx/common-pci.c   |  1 +
 arch/arm/mach-ixp4xx/common.c   | 13 +
 arch/arm/mach-ixp4xx/goramo_mlr.c   |  3 ++-
 arch/arm/mach-ixp4xx/include/mach/debug-macro.S |  4 ++--
 arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 46 
+++--
 arch/arm/mach-ixp4xx/include/mach/qmgr.h| 12 ++--
 arch/arm/mach-ixp4xx/ixp4xx_npe.c   |  9 -
 arch/arm/mach-ixp4xx/ixp4xx_qmgr.c  | 12 +---
 drivers/char/hw_random/Kconfig  |  6 +++---
 drivers/char/hw_random/ixp4xx-rng.c |  5 -
 drivers/crypto/Kconfig  |  2 +-
 drivers/crypto/ixp4xx_crypto.c  | 12 ++--
 drivers/net/wan/ixp4xx_hss.c|  2 +-
 13 files changed, 59 insertions(+), 68 deletions(-)

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

[PULL REQ] IXP4xx changes for Linux 3.7

[Krzysztof Halasa]

Linus,

please pull my ARM IXP4xx changes for 3.7:

The following changes since commit 4d7127dace8cf4b05eb7c8c8531fc204fbb195f4:

Merge branch 'for-linus' of git://git.kernel.org/.../jmorris/linux-security
(2012-10-13 11:29:00 +0900)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/chris/linux.git next

for you to fetch changes up to b94740b3b38fd8e37fcd3bb06a18ec2796061c7d:
  IXP4xx: use __iomem for MMIO (2012-10-13 20:37:30 +0200)

Build-tested for now. This is based on your current tree tip because it
depends on commits following 3.6 release.

Thanks.


Arnd Bergmann (1):
  IXP4xx: use __iomem for MMIO

Krzysztof Hałasa (9):
  IXP4xx: Fix Goramo MultiLink platform compilation.
  IXP4xx: Fix off-by-one bug in Goramo MultiLink platform.
  IXP4xx: HW pseudo-random generator is available on IXP45x/46x only.
  IXP4xx: ixp4xx_crypto driver requires Queue Manager and NPE drivers.
  IXP4xx: Remove time limit for PCI TRDY to enable use of slow devices.
  WAN: Remove redundant HDLC info printed by IXP4xx HSS driver.
  IXP4xx crypto: MOD_AES{128,192,256} already include key size.
  IXP4xx: Always ioremap() Queue Manager MMIO region at boot.
  IXP4xx: map CPU config registers within VMALLOC region.

Tim Gardner (1):
  ixp4xx: Declare MODULE_FIRMWARE usage

 arch/arm/mach-ixp4xx/common-pci.c   |  1 +
 arch/arm/mach-ixp4xx/common.c   | 13 +
 arch/arm/mach-ixp4xx/goramo_mlr.c   |  3 ++-
 arch/arm/mach-ixp4xx/include/mach/debug-macro.S |  4 ++--
 arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 46 
+++--
 arch/arm/mach-ixp4xx/include/mach/qmgr.h| 12 ++--
 arch/arm/mach-ixp4xx/ixp4xx_npe.c   |  9 -
 arch/arm/mach-ixp4xx/ixp4xx_qmgr.c  | 12 +---
 drivers/char/hw_random/Kconfig  |  6 +++---
 drivers/char/hw_random/ixp4xx-rng.c |  5 -
 drivers/crypto/Kconfig  |  2 +-
 drivers/crypto/ixp4xx_crypto.c  | 12 ++--
 drivers/net/wan/ixp4xx_hss.c|  2 +-
 13 files changed, 59 insertions(+), 68 deletions(-)

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

Re: ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

Russell King - ARM Linux  writes:

>> Note I'm not only doing ARM, but also X86 and MIPS, with additional code
>> shared between them, and the "stable" part of it is what I'm paid for.
>> I can't simply work with arm-soc, none of my platforms will even boot
>> with pure arm-soc.
>
> I'm assuming that Linus' -rc kernels work for you, yes?

No, but it's OK :-)

I meant I need additional code for a) ARM and MIPS-based boards (off the
shelf by Ubiquity, Gateworks etc., support ported mostly from OpenWRT),
b) custom hardware used only by us.

I hope to eventually submit a) upstream, then arm-soc and Linus' trees
would work for me. I know it's a mess, but for now I have no options.

BTW the situation with hardware supported only by OpenWRT is a part of
the equation.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

Russell King - ARM Linux li...@arm.linux.org.uk writes:

 Note I'm not only doing ARM, but also X86 and MIPS, with additional code
 shared between them, and the stable part of it is what I'm paid for.
 I can't simply work with arm-soc, none of my platforms will even boot
 with pure arm-soc.

 I'm assuming that Linus' -rc kernels work for you, yes?

No, but it's OK :-)

I meant I need additional code for a) ARM and MIPS-based boards (off the
shelf by Ubiquity, Gateworks etc., support ported mostly from OpenWRT),
b) custom hardware used only by us.

I hope to eventually submit a) upstream, then arm-soc and Linus' trees
would work for me. I know it's a mess, but for now I have no options.

BTW the situation with hardware supported only by OpenWRT is a part of
the equation.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

I realize my work flow is probably suboptimal, though I can't see
an easy solution.

Russell King - ARM Linux  writes:

> That makes no sense what so ever.  The tree which is used for the next
> stable tree will be Linus' tree, which will be the result of merging all
> those other trees into Linus' tree.  If you base your changes off only
> Linus' tree on the run up to the next merge window, then you are preparing
> your changes against an _older_ kernel than if you base them off a tree
> containing the changes which will be _included_ in the next merge
> window.

Yes. I have to fix my tree and ask for pull before the end of merge
window (fixes could follow, but it's best when there are no fixes
needed). I know it's not a great plan. Fortunately I'm not doing heavy,
conflicting development on this.

Note I'm not only doing ARM, but also X86 and MIPS, with additional code
shared between them, and the "stable" part of it is what I'm paid for.
I can't simply work with arm-soc, none of my platforms will even boot
with pure arm-soc.

> If you want to work in total isolation, that's your choice, but in the
> long run you will be playing catch-up rather than being prepared early
> for the changes which will happen at the next merge window.  If you want
> -rc1, rc2, rc3, rc4 etc to be broken on IXP4xx until you get around to
> fixing up the merge window breakage, then go ahead.
>
> Otherwise, participate and be part of the community, and get your changes
> into arm-soc, so that others can see what's going on.

Unfortunately this seems to require more time on my part, time I don't
have. Perhaps I'm missing something?
I.e., I can dedicate some time around the merge window (not every one,
but this may improve), but not much more.

I don't think there are any "others" interested in what's going on on
IXP4xx. Simply, there isn't anything of importance here. It won't cause
severe conflicts (and "next" helps here), and I post my patches to the
list just like all people do.

Of course if you can see a better way (given the limitations), I'm open.

BTW I still think I'm a part of the community, even if I don't submit
code through arm-soc :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

Arnd Bergmann  writes:

>> Could you please point me to a statement requiring eg. my changes to go
>> through arm-soc?
>
> We've been doing it like this for some time. Stephen Warren replied
> to your request to add your tree to linux-next in
>
> http://comments.gmane.org/gmane.linux.kernel/1356118
>
> explaining how it works. Olof sent a mail last week in 
>
> http://lkml.org/lkml/2012/9/21/31
>
> explaining that we're closing the window for 3.7 except for a
> few things that were already submitted earlier.

No offense, but... You say how does it work for YOU but that's not
exactly what I'm asking for. I'm asking for a statement that it's not OK
for me to push my IXP4xx changes straight to Linus.

> The arm-soc process is definitely meant to make your life easier
> as well as help Linus understand what's going on with all of ARM
> to the degree that he needs to know, but it only works if everyone
> participates.

I'd like to know how is it easier for me. Actually, I think it would
only make things worse for everyone (mostly for me). Also, I can't see
how "it only works if everyone participates" is true.

I'm currently supporting (for our internal needs) hw platforms based
on x86, MIPS and now ARM. I'm using 3.1 (non-trivial upgrades required
so -ETIME) and 3.5 "stable" trees, and need to also use Linus' current
tree since it's the next stable. The hw is e.g. Gateworks' platforms
with code taken from e.g. OpenWRT. I hope to have most of this in Linus'
tree when it's eventually ready. Unfortunately, I'm just one man, and
the above is only a slim part of my work. Egoistically, I don't think
I'm currently willing to spend time with arm-soc tree, if I can't see
any real technical benefit to anyone.

It would be different if my tree included e.g. core ARM changes - but it
doesn't. What's the _real_ reason for asking me to push my changes
indirectly?

Also, not that it's the most important, but how is it better for anyone
to delay changes - which are completely orthogonal to arm-soc - for
additional months? Doesn't "release early, release often" make sense
anymore?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

Arnd Bergmann  writes:

> Krzysztof, please apply in your next branch so this problem goes away in
> 3.8.

Right, I'll look at it.

> I realize it won't make 3.7 any more since the base patches are not
> in arm-soc, but it's bad if linux-next is broken.

Well, I'm not aware of any requirement to push my IXP4xx changes
exclusively through arm-soc. I believe I can still send my tree straight
to Linus.

My tree is fairly isolated (with one trivial patch in need of ack from
Russell, but I can remove that) and, to be honest, I can't see any
benefit to anyone, caused by sending through intermediate trees. In
fact, now that I have at last a bit of spare time to work on IXP4xx
again (acquired some IXP435 devices), such requirement would only mean
extra workload to me.

Could you please point me to a statement requiring eg. my changes to go
through arm-soc?

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

ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

Arnd Bergmann a...@arndb.de writes:

 Krzysztof, please apply in your next branch so this problem goes away in
 3.8.

Right, I'll look at it.

 I realize it won't make 3.7 any more since the base patches are not
 in arm-soc, but it's bad if linux-next is broken.

Well, I'm not aware of any requirement to push my IXP4xx changes
exclusively through arm-soc. I believe I can still send my tree straight
to Linus.

My tree is fairly isolated (with one trivial patch in need of ack from
Russell, but I can remove that) and, to be honest, I can't see any
benefit to anyone, caused by sending through intermediate trees. In
fact, now that I have at last a bit of spare time to work on IXP4xx
again (acquired some IXP435 devices), such requirement would only mean
extra workload to me.

Could you please point me to a statement requiring eg. my changes to go
through arm-soc?

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

Re: ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

Arnd Bergmann a...@arndb.de writes:

 Could you please point me to a statement requiring eg. my changes to go
 through arm-soc?

 We've been doing it like this for some time. Stephen Warren replied
 to your request to add your tree to linux-next in

 http://comments.gmane.org/gmane.linux.kernel/1356118

 explaining how it works. Olof sent a mail last week in 

 http://lkml.org/lkml/2012/9/21/31

 explaining that we're closing the window for 3.7 except for a
 few things that were already submitted earlier.

No offense, but... You say how does it work for YOU but that's not
exactly what I'm asking for. I'm asking for a statement that it's not OK
for me to push my IXP4xx changes straight to Linus.

 The arm-soc process is definitely meant to make your life easier
 as well as help Linus understand what's going on with all of ARM
 to the degree that he needs to know, but it only works if everyone
 participates.

I'd like to know how is it easier for me. Actually, I think it would
only make things worse for everyone (mostly for me). Also, I can't see
how it only works if everyone participates is true.

I'm currently supporting (for our internal needs) hw platforms based
on x86, MIPS and now ARM. I'm using 3.1 (non-trivial upgrades required
so -ETIME) and 3.5 stable trees, and need to also use Linus' current
tree since it's the next stable. The hw is e.g. Gateworks' platforms
with code taken from e.g. OpenWRT. I hope to have most of this in Linus'
tree when it's eventually ready. Unfortunately, I'm just one man, and
the above is only a slim part of my work. Egoistically, I don't think
I'm currently willing to spend time with arm-soc tree, if I can't see
any real technical benefit to anyone.

It would be different if my tree included e.g. core ARM changes - but it
doesn't. What's the _real_ reason for asking me to push my changes
indirectly?

Also, not that it's the most important, but how is it better for anyone
to delay changes - which are completely orthogonal to arm-soc - for
additional months? Doesn't release early, release often make sense
anymore?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: ARM SoC tree, Was: Re: [PATCH 05/12] ARM: ixp4xx: use __iomem for MMIO

[Krzysztof Halasa]

I realize my work flow is probably suboptimal, though I can't see
an easy solution.

Russell King - ARM Linux li...@arm.linux.org.uk writes:

 That makes no sense what so ever.  The tree which is used for the next
 stable tree will be Linus' tree, which will be the result of merging all
 those other trees into Linus' tree.  If you base your changes off only
 Linus' tree on the run up to the next merge window, then you are preparing
 your changes against an _older_ kernel than if you base them off a tree
 containing the changes which will be _included_ in the next merge
 window.

Yes. I have to fix my tree and ask for pull before the end of merge
window (fixes could follow, but it's best when there are no fixes
needed). I know it's not a great plan. Fortunately I'm not doing heavy,
conflicting development on this.

Note I'm not only doing ARM, but also X86 and MIPS, with additional code
shared between them, and the stable part of it is what I'm paid for.
I can't simply work with arm-soc, none of my platforms will even boot
with pure arm-soc.

 If you want to work in total isolation, that's your choice, but in the
 long run you will be playing catch-up rather than being prepared early
 for the changes which will happen at the next merge window.  If you want
 -rc1, rc2, rc3, rc4 etc to be broken on IXP4xx until you get around to
 fixing up the merge window breakage, then go ahead.

 Otherwise, participate and be part of the community, and get your changes
 into arm-soc, so that others can see what's going on.

Unfortunately this seems to require more time on my part, time I don't
have. Perhaps I'm missing something?
I.e., I can dedicate some time around the merge window (not every one,
but this may improve), but not much more.

I don't think there are any others interested in what's going on on
IXP4xx. Simply, there isn't anything of importance here. It won't cause
severe conflicts (and next helps here), and I post my patches to the
list just like all people do.

Of course if you can see a better way (given the limitations), I'm open.

BTW I still think I'm a part of the community, even if I don't submit
code through arm-soc :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 08+09/24] ARM: ixp4xx: use __iomem pointers for MMIO

[Krzysztof Halasa]

Actually, there are some minor problems here (both patches merged,
ixp4xx only):

Arnd Bergmann  writes:

> --- a/arch/arm/mach-ixp4xx/include/mach/cpu.h
> +++ b/arch/arm/mach-ixp4xx/include/mach/cpu.h
> @@ -37,7 +38,7 @@
>  
>  static inline u32 ixp4xx_read_feature_bits(void)
>  {
> - u32 val = ~*IXP4XX_EXP_CFG2;
> + u32 val = ~__raw_readl(IXP4XX_EXP_CFG2);
>  

This is all fine, but

> @@ -51,7 +52,7 @@ static inline u32 ixp4xx_read_feature_bits(void)
>  
>  static inline void ixp4xx_write_feature_bits(u32 value)
>  {
> - *IXP4XX_EXP_CFG2 = ~value;
> + __raw_writel(~cpu_to_le32(value), IXP4XX_EXP_CFG2);
>  }

The EXP_CFG2 register is already in host order, no need for
cpu_to_le32() as the replaced code clearly shows.

Can you merge both IXP4xx parts (in the two patches) and fix the above,
please? Thanks.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/24] ARM: ixp4xx: use __iomem pointers for MMIO

[Krzysztof Halasa]

Krzysztof Halasa  writes:

>> At the moment, this patch conflicts with other patches in linux-next,
>> need to sort this out.
>
> Very nice, I will take care of these 3 patches.

I mean, just this one IXP4xx patch :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Another tree for "next"

[Krzysztof Halasa]

Stephen Rothwell  writes:

>> >>> git://git.kernel.org/pub/scm/linux/kernel/git/chris/linux.git  branch: 
>> >>> next

> OK, I have added your tree from today.

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

Re: [PATCH 09/24] ARM: ixp4xx: use __iomem pointers for MMIO

[Krzysztof Halasa]

Arnd Bergmann  writes:

> ARM is moving to stricter checks on readl/write functions,
> so we need to use the correct types everywhere.
>
> At the moment, this patch conflicts with other patches in linux-next,
> need to sort this out.

Very nice, I will take care of these 3 patches.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/24] ARM: ixp4xx: use __iomem pointers for MMIO

[Krzysztof Halasa]

Arnd Bergmann a...@arndb.de writes:

 ARM is moving to stricter checks on readl/write functions,
 so we need to use the correct types everywhere.

 At the moment, this patch conflicts with other patches in linux-next,
 need to sort this out.

Very nice, I will take care of these 3 patches.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Another tree for next

[Krzysztof Halasa]

Stephen Rothwell s...@canb.auug.org.au writes:

  git://git.kernel.org/pub/scm/linux/kernel/git/chris/linux.git  branch: 
  next

 OK, I have added your tree from today.

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

Re: [PATCH 09/24] ARM: ixp4xx: use __iomem pointers for MMIO

[Krzysztof Halasa]

Krzysztof Halasa k...@pm.waw.pl writes:

 At the moment, this patch conflicts with other patches in linux-next,
 need to sort this out.

 Very nice, I will take care of these 3 patches.

I mean, just this one IXP4xx patch :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 08+09/24] ARM: ixp4xx: use __iomem pointers for MMIO

[Krzysztof Halasa]

Actually, there are some minor problems here (both patches merged,
ixp4xx only):

Arnd Bergmann a...@arndb.de writes:

 --- a/arch/arm/mach-ixp4xx/include/mach/cpu.h
 +++ b/arch/arm/mach-ixp4xx/include/mach/cpu.h
 @@ -37,7 +38,7 @@
  
  static inline u32 ixp4xx_read_feature_bits(void)
  {
 - u32 val = ~*IXP4XX_EXP_CFG2;
 + u32 val = ~__raw_readl(IXP4XX_EXP_CFG2);
  

This is all fine, but

 @@ -51,7 +52,7 @@ static inline u32 ixp4xx_read_feature_bits(void)
  
  static inline void ixp4xx_write_feature_bits(u32 value)
  {
 - *IXP4XX_EXP_CFG2 = ~value;
 + __raw_writel(~cpu_to_le32(value), IXP4XX_EXP_CFG2);
  }

The EXP_CFG2 register is already in host order, no need for
cpu_to_le32() as the replaced code clearly shows.

Can you merge both IXP4xx parts (in the two patches) and fix the above,
please? Thanks.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Another tree for "next"

[Krzysztof Halasa]

Stephen Rothwell  writes:

>> could you please add the following to your "next" tree?
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/chris/linux.git  branch: next

> Could you be a bit more clear about what will be in that tree, please?
> Also when you repost this request, please post it a bit more widely among
> other people/lists who may be interested in this work.  Remember that
> linux-next is really only for stuff that is ready for Linus to pull into
> the next merge window (or bug fixes).  Also, would this tree be pulled
> directly by Linus, or via some other tree?

My plan is to merge all the stuff I'm (eventually) working on in the
above tree. For now - mostly IXP4xx (ARM CPU etc.) stuff which goes to
Linus, and (from time to time) WAN network drivers pulled by Dave. BTW
this includes both current fixes (for "this") and true "next" changes.

All of this is low profile and will be frequently empty.

Hope that's ok. Alternatively I can add separate trees/branches
(currently I'm only working on IXP4xx, at least as far as we're talking
about pullable git trees). Just wanted to minimize effort needed on
your side.

Not sure about these other people/lists, added lkml.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Another tree for next

[Krzysztof Halasa]

Stephen Rothwell s...@canb.auug.org.au writes:

 could you please add the following to your next tree?

 git://git.kernel.org/pub/scm/linux/kernel/git/chris/linux.git  branch: next

 Could you be a bit more clear about what will be in that tree, please?
 Also when you repost this request, please post it a bit more widely among
 other people/lists who may be interested in this work.  Remember that
 linux-next is really only for stuff that is ready for Linus to pull into
 the next merge window (or bug fixes).  Also, would this tree be pulled
 directly by Linus, or via some other tree?

My plan is to merge all the stuff I'm (eventually) working on in the
above tree. For now - mostly IXP4xx (ARM CPU etc.) stuff which goes to
Linus, and (from time to time) WAN network drivers pulled by Dave. BTW
this includes both current fixes (for this) and true next changes.

All of this is low profile and will be frequently empty.

Hope that's ok. Alternatively I can add separate trees/branches
(currently I'm only working on IXP4xx, at least as far as we're talking
about pullable git trees). Just wanted to minimize effort needed on
your side.

Not sure about these other people/lists, added lkml.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 3.4.10 N_GSM tty_io WARNING and lockup

[Krzysztof Halasa]

Alan Cox  writes:

>> BTW I'm not sure if it's a known bug: I closed the /dev/ttyS0 with
>> /dev/gsmmuxX still open, and then tried to set N_TTY ldisc on the
>> master device. It didn't work so well :-(
>
> That should be fixed in 3.7. It's unfixable until the lock splitting work
> is merged.

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

Re: 3.4.10 N_GSM tty_io WARNING and lockup

[Krzysztof Halasa]

> In the meantime I've rebased to v3.5.3 and it started to work. Do you
> think it is worth it investigating v3.4.10 at this point?

BTW I'm not sure if it's a known bug: I closed the /dev/ttyS0 with
/dev/gsmmuxX still open, and then tried to set N_TTY ldisc on the
master device. It didn't work so well :-(

# killall -9 microcom   # they use /dev/gsmmux[0-3]
# killall -9 iru# the program messing with ldiscs
# ps axf

  344 ?Ss 0:00  \_ sshd: root@pts/1
  346 pts/1Ss 0:00  |   \_ -sh
  405 pts/1D+ 0:00  |   \_ [microcom]
  347 ?Ss 0:00  \_ sshd: root@pts/2
  349 pts/2Ss 0:00  |   \_ -sh
  403 pts/2D+ 0:00  |   \_ [microcom]
  406 ?Ss 0:00  \_ sshd: root@pts/3
  408 pts/3Ss 0:00  |   \_ -sh
  412 pts/3D+ 0:00  |   \_ [microcom]
  409 ?Ss 0:00  \_ sshd: root@pts/4
  411 pts/4Ss 0:00  \_ -sh
  413 pts/4D+ 0:00  \_ [microcom]
  415 pts/0D  0:00 iru
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 3.4.10 N_GSM tty_io WARNING and lockup

[Krzysztof Halasa]

 In the meantime I've rebased to v3.5.3 and it started to work. Do you
 think it is worth it investigating v3.4.10 at this point?

BTW I'm not sure if it's a known bug: I closed the /dev/ttyS0 with
/dev/gsmmuxX still open, and then tried to set N_TTY ldisc on the
master device. It didn't work so well :-(

# killall -9 microcom   # they use /dev/gsmmux[0-3]
# killall -9 iru# the program messing with ldiscs
# ps axf

  344 ?Ss 0:00  \_ sshd: root@pts/1
  346 pts/1Ss 0:00  |   \_ -sh
  405 pts/1D+ 0:00  |   \_ [microcom]
  347 ?Ss 0:00  \_ sshd: root@pts/2
  349 pts/2Ss 0:00  |   \_ -sh
  403 pts/2D+ 0:00  |   \_ [microcom]
  406 ?Ss 0:00  \_ sshd: root@pts/3
  408 pts/3Ss 0:00  |   \_ -sh
  412 pts/3D+ 0:00  |   \_ [microcom]
  409 ?Ss 0:00  \_ sshd: root@pts/4
  411 pts/4Ss 0:00  \_ -sh
  413 pts/4D+ 0:00  \_ [microcom]
  415 pts/0D  0:00 iru
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: 3.4.10 N_GSM tty_io WARNING and lockup

[Krzysztof Halasa]

Alan Cox a...@lxorguk.ukuu.org.uk writes:

 BTW I'm not sure if it's a known bug: I closed the /dev/ttyS0 with
 /dev/gsmmuxX still open, and then tried to set N_TTY ldisc on the
 master device. It didn't work so well :-(

 That should be fixed in 3.7. It's unfixable until the lock splitting work
 is merged.

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

Re: [PATCH] ixp4xx: Declare MODULE_FIRMWARE usage

[Krzysztof Halasa]

Tim Gardner  writes:

> +++ b/arch/arm/mach-ixp4xx/ixp4xx_npe.c
> @@ -116,7 +116,11 @@
>  /* NPE mailbox_status value for reset */
>  #define RESET_MBOX_STAT  0xF0F0
>  
> -const char *npe_names[] = { "NPE-A", "NPE-B", "NPE-C" };
> +#define NPE_A_FIRMWARE "NPE-A"
> +#define NPE_B_FIRMWARE "NPE-B"
> +#define NPE_C_FIRMWARE "NPE-C"
> +
> +const char *npe_names[] = { NPE_A_FIRMWARE, NPE_B_FIRMWARE, NPE_C_FIRMWARE };
>  
>  #define print_npe(pri, npe, fmt, ...)
> \
>   printk(pri "%s: " fmt, npe_name(npe), ## __VA_ARGS__)
> @@ -724,6 +728,9 @@ module_exit(npe_cleanup_module);
>  
>  MODULE_AUTHOR("Krzysztof Halasa");
>  MODULE_LICENSE("GPL v2");
> +MODULE_FIRMWARE(NPE_A_FIRMWARE);
> +MODULE_FIRMWARE(NPE_B_FIRMWARE);
> +MODULE_FIRMWARE(NPE_C_FIRMWARE);
>  
>  EXPORT_SYMBOL(npe_names);
>  EXPORT_SYMBOL(npe_running);

This looks like a good idea, thanks.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ixp4xx: Declare MODULE_FIRMWARE usage

[Krzysztof Halasa]

Tim Gardner tim.gard...@canonical.com writes:

 +++ b/arch/arm/mach-ixp4xx/ixp4xx_npe.c
 @@ -116,7 +116,11 @@
  /* NPE mailbox_status value for reset */
  #define RESET_MBOX_STAT  0xF0F0
  
 -const char *npe_names[] = { NPE-A, NPE-B, NPE-C };
 +#define NPE_A_FIRMWARE NPE-A
 +#define NPE_B_FIRMWARE NPE-B
 +#define NPE_C_FIRMWARE NPE-C
 +
 +const char *npe_names[] = { NPE_A_FIRMWARE, NPE_B_FIRMWARE, NPE_C_FIRMWARE };
  
  #define print_npe(pri, npe, fmt, ...)
 \
   printk(pri %s:  fmt, npe_name(npe), ## __VA_ARGS__)
 @@ -724,6 +728,9 @@ module_exit(npe_cleanup_module);
  
  MODULE_AUTHOR(Krzysztof Halasa);
  MODULE_LICENSE(GPL v2);
 +MODULE_FIRMWARE(NPE_A_FIRMWARE);
 +MODULE_FIRMWARE(NPE_B_FIRMWARE);
 +MODULE_FIRMWARE(NPE_C_FIRMWARE);
  
  EXPORT_SYMBOL(npe_names);
  EXPORT_SYMBOL(npe_running);

This looks like a good idea, thanks.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] 2.6.25-rc2-mm1 - fix mcount GPL bogosity.

[Krzysztof Halasa]

Alan Cox <[EMAIL PROTECTED]> writes:

>> Actually I think the _GPL exports are really harmful - somebody
>> distributing a binary module may claim he/she doesn't violate the GPL
>> because the module uses only non-GPL exports. OTOH GPL symbols give
>
> They can claim that anyway. The can claim to be alien life forms too. Claim
> is not the same as legal decision.

Sure but it may be now easier to convince the judge they are right. Of
course if they aren't using GPL exports - if they are, perhaps harder.

> From what I've seen its helped make binary
> module abusers more cautious.

Those not using _GPL exports?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] 2.6.25-rc2-mm1 - fix mcount GPL bogosity.

[Krzysztof Halasa]

> One may say _GPL is a strong indication that all users are
> automatically a derivative works, but it's only that - indication. It
> doesn't mean they are really derivative works and it doesn't mean a
> module not using any _GPL exports isn't a derivative.

Actually I think the _GPL exports are really harmful - somebody
distributing a binary module may claim he/she doesn't violate the GPL
because the module uses only non-GPL exports. OTOH GPL symbols give
_us_ exactly nothing.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Tabs, spaces, indent and 80 character lines

[Krzysztof Halasa]

Jan Engelhardt <[EMAIL PROTECTED]> writes:

> Now back to coding, oh and don't forget send a patch for CodingStyle 
> since a mail without one is often taken even less seriously.

Someone with a patch to Emacs to use tabs for ident + spaces for
alignment maybe? :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] 2.6.25-rc2-mm1 - fix mcount GPL bogosity.

[Krzysztof Halasa]

"David Schwartz" <[EMAIL PROTECTED]> writes:

> I don't know who told you that or why, but it's obvious nonsense,

Correct.

> Exports should be marked GPL if and only if they cannot be used
> except in a derivative work. If it is possible to use them without taking
> sufficient protectable expression, they should not be marked GPL.

This isn't very obvious to me.

The licence doesn't talk about GPL or non-GPL exports. It doesn't
restrict the use, only distribution of the software. One is free to
remove _GPL from the code and distribute it anyway (except perhaps for
some DMCA nonsense).

If a code is a derivative work it has to be distributed (use is not
restricted) under GPL, EXPORT _GPL or not _GPL.

One may say _GPL is a strong indication that all users are
automatically a derivative works, but it's only that - indication. It
doesn't mean they are really derivative works and it doesn't mean a
module not using any _GPL exports isn't a derivative.

I think introducing these _GPL symbols was a mistake in the first place.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] 2.6.25-rc2-mm1 - fix mcount GPL bogosity.

[Krzysztof Halasa]

David Schwartz [EMAIL PROTECTED] writes:

 I don't know who told you that or why, but it's obvious nonsense,

Correct.

 Exports should be marked GPL if and only if they cannot be used
 except in a derivative work. If it is possible to use them without taking
 sufficient protectable expression, they should not be marked GPL.

This isn't very obvious to me.

The licence doesn't talk about GPL or non-GPL exports. It doesn't
restrict the use, only distribution of the software. One is free to
remove _GPL from the code and distribute it anyway (except perhaps for
some DMCA nonsense).

If a code is a derivative work it has to be distributed (use is not
restricted) under GPL, EXPORT _GPL or not _GPL.

One may say _GPL is a strong indication that all users are
automatically a derivative works, but it's only that - indication. It
doesn't mean they are really derivative works and it doesn't mean a
module not using any _GPL exports isn't a derivative.

I think introducing these _GPL symbols was a mistake in the first place.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Tabs, spaces, indent and 80 character lines

[Krzysztof Halasa]

Jan Engelhardt [EMAIL PROTECTED] writes:

 Now back to coding, oh and don't forget send a patch for CodingStyle 
 since a mail without one is often taken even less seriously.

Someone with a patch to Emacs to use tabs for ident + spaces for
alignment maybe? :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] 2.6.25-rc2-mm1 - fix mcount GPL bogosity.

[Krzysztof Halasa]

 One may say _GPL is a strong indication that all users are
 automatically a derivative works, but it's only that - indication. It
 doesn't mean they are really derivative works and it doesn't mean a
 module not using any _GPL exports isn't a derivative.

Actually I think the _GPL exports are really harmful - somebody
distributing a binary module may claim he/she doesn't violate the GPL
because the module uses only non-GPL exports. OTOH GPL symbols give
_us_ exactly nothing.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] 2.6.25-rc2-mm1 - fix mcount GPL bogosity.

[Krzysztof Halasa]

Alan Cox [EMAIL PROTECTED] writes:

 Actually I think the _GPL exports are really harmful - somebody
 distributing a binary module may claim he/she doesn't violate the GPL
 because the module uses only non-GPL exports. OTOH GPL symbols give

 They can claim that anyway. The can claim to be alien life forms too. Claim
 is not the same as legal decision.

Sure but it may be now easier to convince the judge they are right. Of
course if they aren't using GPL exports - if they are, perhaps harder.

 From what I've seen its helped make binary
 module abusers more cautious.

Those not using _GPL exports?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Tabs, spaces, indent and 80 character lines

[Krzysztof Halasa]

Richard Knutsson <[EMAIL PROTECTED]> writes:

>> I guess we could use tabs only at the line start, for indentation
>> only. Rather hard to implement, most text editors can't do that yet.
>>   
> You mean for split lines?

Syntactic indentation vs alignment (including comments after
non-blank, values for struct initialization etc, split lines too).

> Hopefully there won't be that many, so there
> is just to delete the tabs it added and replace it with spaces.

Actually tabs "should" be used for indentation at start of the
line, then spaces. "Ideally" :-)

I.e., something like
   if (cond && (cond2 ||
   _cond3))
  do_something();

Underline = space.

Perhaps some day...

> Exactly! But then we can remove the "we use 8 wide tabs in the kernel"
> in CodeStyle.

I'm not sure it's practically possible now.

>> Unpacked sources will be much bigger with not tabs, sure.
>>   
> Without no tabs at all, you mean?

With spaces in place of all tabs.

All tabs converted to spaces = 20% more?
"Alignment" tabs converted to spaces? How cares how much more would it
take if it's the correct thing. Except that it's not very practical at
this point.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Tabs, spaces, indent and 80 character lines

[Krzysztof Halasa]

Richard Knutsson <[EMAIL PROTECTED]> writes:

> Why hinder a developer who prefer
> 2, 4, 6 or any other != 8 width?

I guess we could use tabs only at the line start, for indentation
only. Rather hard to implement, most text editors can't do that yet.

> By only using tabs as indents, and
> changing the CodeStyle to be something like "maximum 80
> characters-wide lines, with a tab-setting of 8 spaces",

This changes nothing.

> that is
> possible + easier to write code-checkers [2].

I doubt it.

> Or are we really that concerned about the disk-space? ;)

Unpacked sources will be much bigger with not tabs, sure.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jörn Engel <[EMAIL PROTECTED]> writes:

> I strongly disagree.  Machine-generated warnings are a great way of
> quickly locating a large amount of questionable code in an otherwise
> overwhelming haystack.  It doesn't even matter much, which warnings you
> look for.  Almost all code checkers find the same hotspots.

I think you misunderstood. Of course I'm not against warnings in
general. I'm rather talking about _authority_ of human vs machine,
in this specific ("measuring" code complexity) case.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jörn Engel [EMAIL PROTECTED] writes:

 I strongly disagree.  Machine-generated warnings are a great way of
 quickly locating a large amount of questionable code in an otherwise
 overwhelming haystack.  It doesn't even matter much, which warnings you
 look for.  Almost all code checkers find the same hotspots.

I think you misunderstood. Of course I'm not against warnings in
general. I'm rather talking about _authority_ of human vs machine,
in this specific (measuring code complexity) case.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Tabs, spaces, indent and 80 character lines

[Krzysztof Halasa]

Richard Knutsson [EMAIL PROTECTED] writes:

 Why hinder a developer who prefer
 2, 4, 6 or any other != 8 width?

I guess we could use tabs only at the line start, for indentation
only. Rather hard to implement, most text editors can't do that yet.

 By only using tabs as indents, and
 changing the CodeStyle to be something like maximum 80
 characters-wide lines, with a tab-setting of 8 spaces,

This changes nothing.

 that is
 possible + easier to write code-checkers [2].

I doubt it.

 Or are we really that concerned about the disk-space? ;)

Unpacked sources will be much bigger with not tabs, sure.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Tabs, spaces, indent and 80 character lines

[Krzysztof Halasa]

Richard Knutsson [EMAIL PROTECTED] writes:

 I guess we could use tabs only at the line start, for indentation
 only. Rather hard to implement, most text editors can't do that yet.
   
 You mean for split lines?

Syntactic indentation vs alignment (including comments after
non-blank, values for struct initialization etc, split lines too).

 Hopefully there won't be that many, so there
 is just to delete the tabs it added and replace it with spaces.

Actually tabs should be used for indentation at start of the
line, then spaces. Ideally :-)

I.e., something like
TAB   if (cond  (cond2 ||
TAB   _cond3))
TAB   TAB   do_something();

Underline = space.

Perhaps some day...

 Exactly! But then we can remove the we use 8 wide tabs in the kernel
 in CodeStyle.

I'm not sure it's practically possible now.

 Unpacked sources will be much bigger with not tabs, sure.
   
 Without no tabs at all, you mean?

With spaces in place of all tabs.

All tabs converted to spaces = 20% more?
Alignment tabs converted to spaces? How cares how much more would it
take if it's the correct thing. Except that it's not very practical at
this point.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Pavel Machek <[EMAIL PROTECTED]> writes:

>> Come on, are you doing Linux kernel development on PDA?
>
> I review patches on it, sometimes, yes.

I take it the "sometimes" is the key word :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Pavel Machek [EMAIL PROTECTED] writes:

 Come on, are you doing Linux kernel development on PDA?

 I review patches on it, sometimes, yes.

I take it the sometimes is the key word :-)
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Peter Zijlstra <[EMAIL PROTECTED]> writes:

> So, yes, I have the screen estate for very long lines, but I find that
> long lines require more effort to read (that very much includes leading
> whitespace). Also, since long lines are rare (and they should be, if you
> nest too deep you have other issues) accommodating them would waste a
> lot of screen estate otherwise useful for another column of text.

Either they are rare and you can wrap them and still use 80 columns,
or it turns out they are not so rare and you may want to use wider
windows (not necessarily 132 but perhaps 100).

I think the question isn't if they are rare or not, or if people have
3 * 1920 pixels/line or just 1280.

The question is: is the code more readable with hard limit equal to 80
characters, or maybe is it better to limit code block complexity
instead, and let the maximum number of those small pictures in a line
alone? (Limiting at 132 would have technical sense IMHO).

Better code readability = less bugs without any additional
effort.

> Even with e-mail, I can easily show over 200 characters wide with a
> large font (say 11pt) but find it harder to read emails that don't
> nicely wrap at 78.

Sure - because email is not C code.

Actually you don't "read" C code, word by word, as you read books - do
you?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Pavel Machek <[EMAIL PROTECTED]> writes:

> Zaurus is one example, second is small screen where you need big font
> to keep it readable (x60 on desk).

Come on, are you doing Linux kernel development on PDA?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Linus Torvalds <[EMAIL PROTECTED]> writes:

> Will people then try to fake things out by using 4-space indents 
> and then "deep" indentations will look like just a couple of tabs?)

There is no point in faking it as it's only advisory, it's to help the
author who should be free to ignore the advice. People upstream won't
be fooled by some cheap tab tricks I guess.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Al Viro <[EMAIL PROTECTED]> writes:

> IMO the line length overruns make good warnings.  Not as in "here's a cheap
> way to get more changesets", but as in "that code might have other problems
> nearby" kind of heuristics.

Sure, it does. However the human looking at the code is far better at
spotting such problems. Machine-generated warnings are great when the
machine is actually better than human.

Anyway, warnings are one thing and line limit is another. We may raise
the limit leaving the 80-chars warning in place. Unless there are too
many false positives, of course.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Al Viro [EMAIL PROTECTED] writes:

 IMO the line length overruns make good warnings.  Not as in here's a cheap
 way to get more changesets, but as in that code might have other problems
 nearby kind of heuristics.

Sure, it does. However the human looking at the code is far better at
spotting such problems. Machine-generated warnings are great when the
machine is actually better than human.

Anyway, warnings are one thing and line limit is another. We may raise
the limit leaving the 80-chars warning in place. Unless there are too
many false positives, of course.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Pavel Machek [EMAIL PROTECTED] writes:

 Zaurus is one example, second is small screen where you need big font
 to keep it readable (x60 on desk).

Come on, are you doing Linux kernel development on PDA?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Linus Torvalds [EMAIL PROTECTED] writes:

 Will people then try to fake things out by using 4-space indents 
 and then deep indentations will look like just a couple of tabs?)

There is no point in faking it as it's only advisory, it's to help the
author who should be free to ignore the advice. People upstream won't
be fooled by some cheap tab tricks I guess.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [ofa-general] Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Peter Zijlstra [EMAIL PROTECTED] writes:

 So, yes, I have the screen estate for very long lines, but I find that
 long lines require more effort to read (that very much includes leading
 whitespace). Also, since long lines are rare (and they should be, if you
 nest too deep you have other issues) accommodating them would waste a
 lot of screen estate otherwise useful for another column of text.

Either they are rare and you can wrap them and still use 80 columns,
or it turns out they are not so rare and you may want to use wider
windows (not necessarily 132 but perhaps 100).

I think the question isn't if they are rare or not, or if people have
3 * 1920 pixels/line or just 1280.

The question is: is the code more readable with hard limit equal to 80
characters, or maybe is it better to limit code block complexity
instead, and let the maximum number of those small pictures in a line
alone? (Limiting at 132 would have technical sense IMHO).

Better code readability = less bugs without any additional
effort.

 Even with e-mail, I can easily show over 200 characters wide with a
 large font (say 11pt) but find it harder to read emails that don't
 nicely wrap at 78.

Sure - because email is not C code.

Actually you don't read C code, word by word, as you read books - do
you?
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Al Viro <[EMAIL PROTECTED]> writes:

> ... if your style is lousy.  I agree that situation with printks is
> not normal in that respect and I certainly have no love for the
> checkpatch nonsense, but pressure to keep the fucking nesting depth
> low is a Good Thing(tm).

Indeed. Unfortunately it is orthogonal to the line length limit.

We should limit the nesting level, though I think there is no
universally good value. What is good for one case (a function with a
short multi-level if/for/etc) is bad for another (a long switch()
where any added complexity makes it unparseable).

So I think it just have to meet the author's and reviewers' taste. We
already depend on this.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jeff Garzik <[EMAIL PROTECTED]> writes:

> Every time this discussion comes up, people point out that it remains
> highly common to open multiple 80-column terminal windows, making the
> 80-column limit still highly relevant in modern times.

I guess only because of the limit :-)
Raise the limit, terminal windows will follow.
I'm using 80-column windows, too.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jeff Garzik <[EMAIL PROTECTED]> writes:

> If a driver is full of lines of length >80, that's a problem.

I'm not sure.
We all have more than 80-chars wide displays for years, don't we? The
problem is not the number of characters but code which is too
complex and which may sometimes have too many levels of indentation.

Unfortunately expressing code complexity in terms of line lengths
doesn't seem to work at all.

The 80-chars limit harms development, it makes the code less readable,
sometimes far less readable.

I think we should increase length limit to 132 for the whole kernel
code. Obviously printk() _output_ etc. should stay at 80.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jeff Garzik [EMAIL PROTECTED] writes:

 If a driver is full of lines of length 80, that's a problem.

I'm not sure.
We all have more than 80-chars wide displays for years, don't we? The
problem is not the number of characters but code which is too
complex and which may sometimes have too many levels of indentation.

Unfortunately expressing code complexity in terms of line lengths
doesn't seem to work at all.

The 80-chars limit harms development, it makes the code less readable,
sometimes far less readable.

I think we should increase length limit to 132 for the whole kernel
code. Obviously printk() _output_ etc. should stay at 80.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Jeff Garzik [EMAIL PROTECTED] writes:

 Every time this discussion comes up, people point out that it remains
 highly common to open multiple 80-column terminal windows, making the
 80-column limit still highly relevant in modern times.

I guess only because of the limit :-)
Raise the limit, terminal windows will follow.
I'm using 80-column windows, too.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Merging of completely unreviewed drivers

[Krzysztof Halasa]

Al Viro [EMAIL PROTECTED] writes:

 ... if your style is lousy.  I agree that situation with printks is
 not normal in that respect and I certainly have no love for the
 checkpatch nonsense, but pressure to keep the fucking nesting depth
 low is a Good Thing(tm).

Indeed. Unfortunately it is orthogonal to the line length limit.

We should limit the nesting level, though I think there is no
universally good value. What is good for one case (a function with a
short multi-level if/for/etc) is bad for another (a long switch()
where any added complexity makes it unparseable).

So I think it just have to meet the author's and reviewers' taste. We
already depend on this.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Handshaking on USB serial devices

[Krzysztof Halasa]

Gene Heskett <[EMAIL PROTECTED]> writes:

> Apparently they do not Alan, the pl2303 in particular is a problem child, 
> throwing several lost com errors a day when doing nothing more strenuous than 
> talking to my belkin UPS from apcupsd, very small packets there, 20 bytes I 
> believe at several second intervals.

Is your UPS using "modern" hardware handshaking (CTS = PC can send,
with RTS practically always asserted)? Or perhaps the "old",
"half-duplex" V.24-style (RTS asserted before TX and then waiting for
CTS)?

Are you sure it uses hw handshaking at all? Most (all?) UPSes I used
had only TxD and RxD (for RS-232).
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Handshaking on USB serial devices

[Krzysztof Halasa]

Gene Heskett [EMAIL PROTECTED] writes:

 Apparently they do not Alan, the pl2303 in particular is a problem child, 
 throwing several lost com errors a day when doing nothing more strenuous than 
 talking to my belkin UPS from apcupsd, very small packets there, 20 bytes I 
 believe at several second intervals.

Is your UPS using modern hardware handshaking (CTS = PC can send,
with RTS practically always asserted)? Or perhaps the old,
half-duplex V.24-style (RTS asserted before TX and then waiting for
CTS)?

Are you sure it uses hw handshaking at all? Most (all?) UPSes I used
had only TxD and RxD (for RS-232).
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: E1000 (PCI-E) doesn't work on nforce430, MSI issue.

[Krzysztof Halasa]

"Kok, Auke" <[EMAIL PROTECTED]> writes:

>>> Don't work by default. "pci=nomsi" fixes the problem.
>
> actually does not fix anything - it just works around it by falling
> back to legacy interrupts.

Actually it does, fixes the problem by working around a bug :-)

Thanks for info.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

E1000 (PCI-E) doesn't work on nforce430, MSI issue.

[Krzysztof Halasa]

Hi,

Is it a known problem?
Linux 2.6.24.2, ASUS M2NPV-MX mobo, nforce 430 based, two PCI-E x1
E1000 cards, 32-bit kernel, default e1000 driver (PCI IDs disabled in
e1000e).

Don't work by default. "pci=nomsi" fixes the problem.

82572EI Gigabit Ethernet Controller (Copper) (rev 06)
Subsystem: PRO/1000 PT Desktop Adapter

(8086:10b9 subsystem 8086:1083)

grep eth0 /proc/interrupts 
 20:   1945  1   IO-APIC-fasteoi   eth0

(without "pci=nomsi":
221:  0  0   PCI-MSI-edge  eth0)

other devices of possible interest:
00:00.0 RAM memory: nVidia Corporation C51 Host Bridge (rev a2)
00:00.1 RAM memory: nVidia Corporation C51 Memory Controller 0 (rev a2)
00:00.2 RAM memory: nVidia Corporation C51 Memory Controller 1 (rev a2)
00:00.3 RAM memory: nVidia Corporation C51 Memory Controller 5 (rev a2)
00:00.4 RAM memory: nVidia Corporation C51 Memory Controller 4 (rev a2)
00:00.5 RAM memory: nVidia Corporation C51 Host Bridge (rev a2)
00:00.6 RAM memory: nVidia Corporation C51 Memory Controller 3 (rev a2)
00:00.7 RAM memory: nVidia Corporation C51 Memory Controller 2 (rev a2)
00:03.0 PCI bridge: nVidia Corporation C51 PCI Express Bridge (rev a1)
00:04.0 PCI bridge: nVidia Corporation C51 PCI Express Bridge (rev a1)
00:09.0 RAM memory: nVidia Corporation MCP51 Host Bridge (rev a2)
00:0a.0 ISA bridge: nVidia Corporation MCP51 LPC Bridge (rev a3)
00:0a.1 SMBus: nVidia Corporation MCP51 SMBus (rev a3)
00:0a.2 RAM memory: nVidia Corporation MCP51 Memory Controller 0 (rev a3)
00:10.0 PCI bridge: nVidia Corporation MCP51 PCI Bridge (rev a2)

Additional details on request.
-- 
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/