Re: [PATCH] Staging: speakup: synth.c: removed a space
You and I generally agree on style preferences... I think the warning should be limited to grep ;$. I did a grep on the kernel for ' ;' and found 8000 results. 6000 of them are caught by my semicolon before the newline rule. The remaining 2000 are assembly, macros, and crappy for loops. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/11] staging: et131x: simplify rx dma code
On Wed, Dec 04, 2013 at 03:24:18PM +0800, ZHAO Gang wrote: The original code allocate rx dma memory in several dma_alloc_coherent calls, which causes some problems: 1. since dma_alloc_coherent allocate at least one page memory, it wastes some memory when allocation size is smaller than one page. 2. it causes et131x_rx_dma_memory_free as complex as et131x_rx_dma_memory_alloc Instead, allocate all rx dma memory in one dma_alloc_coherent call makes less code, makes it easy to handle dma allocation error, and makes et131x_rx_dma_memory_free as simple as it could be. Signed-off-by: ZHAO Gang gamer...@gmail.com --- drivers/staging/et131x/et131x.c | 202 1 file changed, 59 insertions(+), 143 deletions(-) Hi Zhao, I haven't had time to look at these last few patches in detail yet, but I do have some comments - The code is an improvement in that it reduces the number of lines of code, but you're also making the driver more susceptible to memory allocation issues, as you're asking for a single large block of memory and not several smaller ones. I'm not sure if that will have an appreciable impact yet. Also, I think a better way to do this allocation would be to create a single struct with the items to be allocated, where as few as possible of the struct members are of dynamic size (drivers/net/ethernet/cirrus/ep93xx_eth.c is a nice example of this) - although this reason alone would not be enough to reject the alloc patches, but the previous reason might. Cheers, Mark ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 18/51] staging: comedi: ni_at_a2150: tidy up the irq request
On 2013-12-03 19:07, H Hartley Sweeten wrote:
The irq is only needed to support async commands. Tidy up the
code that does the request_irq() and remove the noise.
Only hookup the subdevice command support if the irq is available.
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
drivers/staging/comedi/drivers/ni_at_a2150.c | 45 +---
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_at_a2150.c
b/drivers/staging/comedi/drivers/ni_at_a2150.c
index cc69dde..8924fbd 100644
--- a/drivers/staging/comedi/drivers/ni_at_a2150.c
+++ b/drivers/staging/comedi/drivers/ni_at_a2150.c
@@ -689,7 +689,6 @@ static int a2150_attach(struct comedi_device *dev, struct
comedi_devconfig *it)
const struct a2150_board *thisboard = comedi_board(dev);
struct a2150_private *devpriv;
struct comedi_subdevice *s;
- unsigned int irq = it-options[1];
unsigned int dma = it-options[2];
static const int timeout = 2000;
int i;
@@ -703,21 +702,20 @@ static int a2150_attach(struct comedi_device *dev, struct
comedi_devconfig *it)
if (ret)
return ret;
- /* grab our IRQ */
- if (irq) {
- /* check that irq is supported */
- if (irq 3 || irq == 8 || irq == 13 || irq 15) {
- printk( invalid irq line %u\n, irq);
- return -EINVAL;
- }
- if (request_irq(irq, a2150_interrupt, 0,
- dev-driver-driver_name, dev)) {
- printk(unable to allocate irq %u\n, irq);
- return -EINVAL;
+ dev-board_ptr = a2150_boards + a2150_probe(dev);
+ thisboard = comedi_board(dev);
+ dev-board_name = thisboard-name;
+
+ /* only irqs 3, 4, 5, 6, 7, 9, 10, 11, 12, 14 and 15 are valid */
+ if ((1 it-options[1]) 0xdef8) {
This test result is undefined if it-options[1] is greater than or equal
to 32, or less than 0. So you need to range check it as well. You
might as well use the original test since it's easier to understand.
+ ret = request_irq(it-options[1], a2150_interrupt, 0,
+ dev-board_name, dev);
+ if (ret == 0) {
+ dev-irq = it-options[1];
+ devpriv-irq_dma_bits |= IRQ_LVL_BITS(dev-irq);
}
- devpriv-irq_dma_bits |= IRQ_LVL_BITS(irq);
- dev-irq = irq;
}
+
/* initialize dma */
if (dma) {
if (dma == 4 || dma 7) {
@@ -740,27 +738,26 @@ static int a2150_attach(struct comedi_device *dev, struct
comedi_devconfig *it)
devpriv-irq_dma_bits |= DMA_CHAN_BITS(dma);
}
- dev-board_ptr = a2150_boards + a2150_probe(dev);
- thisboard = comedi_board(dev);
- dev-board_name = thisboard-name;
-
ret = comedi_alloc_subdevices(dev, 1);
if (ret)
return ret;
/* analog input subdevice */
s = dev-subdevices[0];
- dev-read_subdev = s;
s-type = COMEDI_SUBD_AI;
- s-subdev_flags = SDF_READABLE | SDF_GROUND | SDF_OTHER | SDF_CMD_READ;
+ s-subdev_flags = SDF_READABLE | SDF_GROUND | SDF_OTHER;
s-n_chan = 4;
- s-len_chanlist = 4;
s-maxdata = 0x;
s-range_table = range_a2150;
- s-do_cmd = a2150_ai_cmd;
- s-do_cmdtest = a2150_ai_cmdtest;
s-insn_read = a2150_ai_rinsn;
- s-cancel = a2150_cancel;
+ if (dev-irq) {
+ dev-read_subdev = s;
+ s-subdev_flags |= SDF_CMD_READ;
+ s-len_chanlist = 4;
+ s-do_cmd = a2150_ai_cmd;
+ s-do_cmdtest = a2150_ai_cmdtest;
+ s-cancel = a2150_cancel;
+ }
/* need to do this for software counting of completed conversions, to
* prevent hardware count from stopping acquisition */
--
-=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk)=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 19/51] staging: comedi: me4000: refactor request_irq() during attach
On 2013-12-03 19:07, H Hartley Sweeten wrote:
Do the request_irq() before setting up the subdevices. This removes an
indent level and makes the code a bit cleaner.
Also, remove the dev_warn() noise about the irq.
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
drivers/staging/comedi/drivers/me4000.c | 29 +
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/comedi/drivers/me4000.c
b/drivers/staging/comedi/drivers/me4000.c
index 3d12e91..b7284d0 100644
--- a/drivers/staging/comedi/drivers/me4000.c
+++ b/drivers/staging/comedi/drivers/me4000.c
@@ -1505,6 +1505,13 @@ static int me4000_auto_attach(struct comedi_device *dev,
me4000_reset(dev);
+ if (pcidev-irq 0) {
+ result = request_irq(pcidev-irq, me4000_ai_isr, IRQF_SHARED,
+ dev-board_name, dev);
+ if (result == 0)
+ dev-irq = pcidev-irq;
+ }
+
result = comedi_alloc_subdevices(dev, 4);
This will cause a NULL pointer dereference on the following line in
me4000_ai_isr():
struct comedi_subdevice *s = dev-subdevices[0];
However, the bug gets fixed by your PATCH 20/51. (The later `if
(!dev-attached) return IRQ_NONE;` in the interrupt handler ensures it
does no further harm.)
if (result)
return result;
@@ -1525,22 +1532,12 @@ static int me4000_auto_attach(struct comedi_device *dev,
s-range_table = me4000_ai_range;
s-insn_read = me4000_ai_insn_read;
- if (pcidev-irq 0) {
- if (request_irq(pcidev-irq, me4000_ai_isr,
- IRQF_SHARED, dev-board_name, dev)) {
- dev_warn(dev-class_dev,
- request_irq failed\n);
- } else {
- dev-read_subdev = s;
- s-subdev_flags |= SDF_CMD_READ;
- s-cancel = me4000_ai_cancel;
- s-do_cmdtest = me4000_ai_do_cmd_test;
- s-do_cmd = me4000_ai_do_cmd;
-
- dev-irq = pcidev-irq;
- }
- } else {
- dev_warn(dev-class_dev, No interrupt available\n);
+ if (dev-irq) {
+ dev-read_subdev = s;
+ s-subdev_flags |= SDF_CMD_READ;
+ s-cancel = me4000_ai_cancel;
+ s-do_cmdtest = me4000_ai_do_cmd_test;
+ s-do_cmd = me4000_ai_do_cmd;
}
} else {
s-type = COMEDI_SUBD_UNUSED;
--
-=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk)=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 22/51] staging: comedi: das1800: tidy up irq request
On 2013-12-03 19:07, H Hartley Sweeten wrote:
Clean up the irq request in the attach of this driver and remove the
dev_{level} noise.
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
drivers/staging/comedi/drivers/das1800.c | 64 ++--
1 file changed, 27 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/comedi/drivers/das1800.c
b/drivers/staging/comedi/drivers/das1800.c
index 1880038..a913787 100644
--- a/drivers/staging/comedi/drivers/das1800.c
+++ b/drivers/staging/comedi/drivers/das1800.c
@@ -1488,7 +1488,6 @@ static int das1800_attach(struct comedi_device *dev,
const struct das1800_board *thisboard = comedi_board(dev);
struct das1800_private *devpriv;
struct comedi_subdevice *s;
- unsigned int irq = it-options[1];
unsigned int dma0 = it-options[2];
unsigned int dma1 = it-options[3];
int board;
@@ -1522,43 +1521,34 @@ static int das1800_attach(struct comedi_device *dev,
devpriv-iobase2 = iobase2;
}
- /* grab our IRQ */
- if (irq) {
- if (request_irq(irq, das1800_interrupt, 0,
- dev-driver-driver_name, dev)) {
- dev_dbg(dev-class_dev, unable to allocate irq %u\n,
- irq);
- return -EINVAL;
- }
- }
- dev-irq = irq;
+ /* only irqs 3, 5, 7, 10, 11, and 15 are valid */
+ if ((1 it-options[1]) 0x8ca8) {
Again, that test is only reliable if it-options[1] is greater than or
equal to 0 and less than 32.
+ ret = request_irq(it-options[1], das1800_interrupt, 0,
+ dev-board_name, dev);
+ if (ret == 0) {
+ dev-irq = it-options[1];
- /* set bits that tell card which irq to use */
- switch (irq) {
- case 0:
- break;
- case 3:
- devpriv-irq_dma_bits |= 0x8;
- break;
- case 5:
- devpriv-irq_dma_bits |= 0x10;
- break;
- case 7:
- devpriv-irq_dma_bits |= 0x18;
- break;
- case 10:
- devpriv-irq_dma_bits |= 0x28;
- break;
- case 11:
- devpriv-irq_dma_bits |= 0x30;
- break;
- case 15:
- devpriv-irq_dma_bits |= 0x38;
- break;
- default:
- dev_err(dev-class_dev, irq out of range\n);
- return -EINVAL;
- break;
+ switch (dev-irq) {
+ case 3:
+ devpriv-irq_dma_bits |= 0x8;
+ break;
+ case 5:
+ devpriv-irq_dma_bits |= 0x10;
+ break;
+ case 7:
+ devpriv-irq_dma_bits |= 0x18;
+ break;
+ case 10:
+ devpriv-irq_dma_bits |= 0x28;
+ break;
+ case 11:
+ devpriv-irq_dma_bits |= 0x30;
+ break;
+ case 15:
+ devpriv-irq_dma_bits |= 0x38;
+ break;
+ }
+ }
}
ret = das1800_init_dma(dev, dma0, dma1);
--
-=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk)=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 49/51] staging: comedi: ni_pcidio: request_irq() before seting up subdevices
On 2013-12-03 19:07, H Hartley Sweeten wrote:
Do the request_irq() before setting up the subdevices. Only hook up
the command support of the irq was sucessfully requested.
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
drivers/staging/comedi/drivers/ni_pcidio.c | 38 +++---
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_pcidio.c
b/drivers/staging/comedi/drivers/ni_pcidio.c
index 9c2bd6c..281665d 100644
--- a/drivers/staging/comedi/drivers/ni_pcidio.c
+++ b/drivers/staging/comedi/drivers/ni_pcidio.c
@@ -1011,6 +1011,14 @@ static int nidio_auto_attach(struct comedi_device *dev,
nidio_reset_board(dev);
+ irq = mite_irq(devpriv-mite);
+ if (irq) {
+ ret = request_irq(irq, nidio_interrupt, IRQF_SHARED,
+ dev-board_name, dev);
+ if (ret == 0)
+ dev-irq = irq;
+ }
+
This results in a couple of NULL dereferences in nidio_interrupt():
struct comedi_subdevice *s = dev-subdevices[0];
struct comedi_async *async = s-async;
The first one would be fixed by your PATCH 50/51, but the other one isn't.
The easiest fix would be to request the IRQ after allocating the subdevices.
ret = comedi_alloc_subdevices(dev, 1);
if (ret)
return ret;
@@ -1019,31 +1027,23 @@ static int nidio_auto_attach(struct comedi_device *dev,
readb(devpriv-mite-daq_io_addr + Chip_Version));
s = dev-subdevices[0];
-
- dev-read_subdev = s;
s-type = COMEDI_SUBD_DIO;
- s-subdev_flags =
- SDF_READABLE | SDF_WRITABLE | SDF_LSAMPL | SDF_PACKED |
- SDF_CMD_READ;
+ s-subdev_flags = SDF_READABLE | SDF_WRITABLE | SDF_LSAMPL | SDF_PACKED;
s-n_chan = 32;
s-range_table = range_digital;
s-maxdata = 1;
s-insn_config = ni_pcidio_insn_config;
s-insn_bits = ni_pcidio_insn_bits;
- s-do_cmd = ni_pcidio_cmd;
- s-do_cmdtest = ni_pcidio_cmdtest;
- s-cancel = ni_pcidio_cancel;
- s-len_chanlist = 32;/* XXX */
- s-buf_change = ni_pcidio_change;
- s-async_dma_dir = DMA_BIDIRECTIONAL;
- s-poll = ni_pcidio_poll;
-
- irq = mite_irq(devpriv-mite);
- if (irq) {
- ret = request_irq(irq, nidio_interrupt, IRQF_SHARED,
- dev-board_name, dev);
- if (ret == 0)
- dev-irq = irq;
+ if (dev-irq) {
+ dev-read_subdev = s;
+ s-subdev_flags |= SDF_CMD_READ;
+ s-async_dma_dir = DMA_BIDIRECTIONAL;
+ s-len_chanlist = s-n_chan;
+ s-do_cmd = ni_pcidio_cmd;
+ s-do_cmdtest = ni_pcidio_cmdtest;
+ s-cancel = ni_pcidio_cancel;
+ s-poll = ni_pcidio_poll;
+ s-buf_change = ni_pcidio_change;
}
return 0;
--
-=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk)=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 27/51] staging: comedi: adl_pci9111: fix incorrect irq passed to request_irq()
On 2013-12-03 19:07, H Hartley Sweeten wrote:
The dev-irq passed to request_irq() will always be 0 when the auto_attach
function is called. The pcidev-irq should be used instead to get the correct
irq number.
It looks like this bug was introduced in the 3.7 kernel. I'll check it
for applicability to the stable kernels when I'm a bit less busy!
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
drivers/staging/comedi/drivers/adl_pci9111.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/comedi/drivers/adl_pci9111.c
b/drivers/staging/comedi/drivers/adl_pci9111.c
index eab8da2..a270a86 100644
--- a/drivers/staging/comedi/drivers/adl_pci9111.c
+++ b/drivers/staging/comedi/drivers/adl_pci9111.c
@@ -859,7 +859,7 @@ static int pci9111_auto_attach(struct comedi_device *dev,
pci9111_reset(dev);
if (pcidev-irq 0) {
- ret = request_irq(dev-irq, pci9111_interrupt,
+ ret = request_irq(pcidev-irq, pci9111_interrupt,
IRQF_SHARED, dev-board_name, dev);
if (ret)
return ret;
--
-=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk)=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/51] staging: comedi: cleanup irq requests
On 2013-12-03 19:07, H Hartley Sweeten wrote: The comedi subsystem only requires the drivers to support interrupts if one or more of the subdevices support async commands. Since this is optional: 1) don't fail the attach if the irq is not available 2) only hookup the async command support if the irq is available 3) remove any async command init from subdevices that don't need it 4) remove any unnecessary sanity checks in the async command functions Make use of the comedi_device 'read_subdev' and 'write_subdev' pointers instead of accessing the dev-subdevices array directly with magic numbers to the correct subdevice. Also, remove any unnecessary debug noise associated with the irq requests. H Hartley Sweeten (51): staging: comedi: s626: fix async command hookup staging: comedi: pcl816: remove 'irq_free' from private data staging: comedi: pcl816: only init command support if irq is available staging: comedi: pcl816: remove 'sub_ai' from private data staging: comedi: pcl816: use dev-read_subdev staging: comedi: pcl818: remove 'irq_free' from private data staging: comedi: pcl818: remove unnecessary 'dev-irq' tests staging: comedi: pcl818: remove function trace noise staging: comedi: pcl818: only init async command members when needed staging: comedi: pcl818: remove 'sub_ai' from private data staging: comedi: pcl818: use dev-read_subdev staging: comedi: pcl818: remove unnecessary s-len_chanlist init staging: comedi: pcl812: remove unnecessary s-len_chanlist init staging: comedi: pcl812: only init async command members when needed staging: comedi: pcl812: use dev-read_subdev staging: comedi: ni_pcimio: tidy up the irq request staging: comedi: ni_pcidio: tidy up the irq request staging: comedi: ni_at_a2150: tidy up the irq request staging: comedi: me4000: refactor request_irq() during attach staging: comedi: me4000: use dev-read_subdev staging: comedi: me4000: remove unnecessary check in the irq handler staging: comedi: das1800: tidy up irq request staging: comedi: das1800: only init command support if irq is available staging: comedi: das1800: remove unnecessary 'dev-irq' test staging: comedi: das1800: use dev-read_subdev staging: comedi: das16m1: remove unnecessary 'dev-irq' test staging: comedi: adl_pci9111: fix incorrect irq passed to request_irq() staging: comedi: adl_pci9111: the irq is only needed for async command support staging: comedi: adl_pci9111: remove unnecessary 'dev-irq' test staging: comedi: dt2814: use dev-read_subdev staging: comedi: dt282x: use dev-read_subdev staging: comedi: dt282x: use dev-write_subdev staging: comedi: amplc_pci230: tidy up irq request staging: comedi: adl_pci9118: tidy up irq request staging: comedi: adv_pci1710: only init async command members when needed staging: comedi: adv_pci1710: use dev-read_subdev staging: comedi: dt3000: don't fail attach if irq is not available staging: comedi: dt3000: use dev-read_subdev staging: comedi: s626: use dev-read_subdev staging: comedi: hwrdv_apci3120: use dev-read_subdev staging: comedi: hwrdv_apci3200: use dev-read_subdev staging: comedi: adl_pci9118: use dev-read_subdev staging: comedi: amplc_pc236: use dev-read_subdev staging: comedi: amplc_pci224: use dev-write_subdev staging: comedi: ni_65xx: use dev-read_subdev staging: comedi: ni_atmio16d: use dev-read_subdev staging: comedi: rtd520: use dev-read_subdev staging: comedi: ni_pcidio: factor board reset out of attach staging: comedi: ni_pcidio: request_irq() before seting up subdevices staging: comedi: ni_pcidio: use dev-read_subdev staging: comedi: multiq3: pass subdevice to encoder_reset() .../comedi/drivers/addi-data/hwdrv_apci3120.c | 6 +- .../comedi/drivers/addi-data/hwdrv_apci3200.c | 2 +- drivers/staging/comedi/drivers/adl_pci9111.c | 31 drivers/staging/comedi/drivers/adl_pci9118.c | 45 +-- drivers/staging/comedi/drivers/adv_pci1710.c | 10 +-- drivers/staging/comedi/drivers/amplc_pc236.c | 2 +- drivers/staging/comedi/drivers/amplc_pci224.c | 2 +- drivers/staging/comedi/drivers/amplc_pci230.c | 27 +++ drivers/staging/comedi/drivers/das16m1.c | 5 -- drivers/staging/comedi/drivers/das1800.c | 89 +- drivers/staging/comedi/drivers/dt2814.c| 4 +- drivers/staging/comedi/drivers/dt282x.c| 10 +-- drivers/staging/comedi/drivers/dt3000.c| 29 +++ drivers/staging/comedi/drivers/me4000.c| 37 - drivers/staging/comedi/drivers/multiq3.c | 6 +- drivers/staging/comedi/drivers/ni_65xx.c | 2 +- drivers/staging/comedi/drivers/ni_at_a2150.c | 45 +-- drivers/staging/comedi/drivers/ni_atmio16d.c | 2 +- drivers/staging/comedi/drivers/ni_pcidio.c | 66
Re: [PATCH 08/11] staging: et131x: simplify rx dma code
On Wed, Dec 4, 2013 at 5:05 PM, Mark Einon mark.ei...@gmail.com wrote: On Wed, Dec 04, 2013 at 03:24:18PM +0800, ZHAO Gang wrote: The original code allocate rx dma memory in several dma_alloc_coherent calls, which causes some problems: 1. since dma_alloc_coherent allocate at least one page memory, it wastes some memory when allocation size is smaller than one page. 2. it causes et131x_rx_dma_memory_free as complex as et131x_rx_dma_memory_alloc Instead, allocate all rx dma memory in one dma_alloc_coherent call makes less code, makes it easy to handle dma allocation error, and makes et131x_rx_dma_memory_free as simple as it could be. Signed-off-by: ZHAO Gang gamer...@gmail.com --- drivers/staging/et131x/et131x.c | 202 1 file changed, 59 insertions(+), 143 deletions(-) Hi Zhao, I haven't had time to look at these last few patches in detail yet, but I do have some comments - The code is an improvement in that it reduces the number of lines of code, but you're also making the driver more susceptible to memory allocation issues, as you're asking for a single large block of memory and not several smaller ones. I'm not sure if that will have an appreciable impact yet. The first motivation that I want to change the dma alloc code is to add error check code, then I find it's not trivial to add it, since there are several dma_alloc_coherent, some in for loop, if dma alloc failed in the middle, code to free already allocated dma memery will be hard to read, so I think combine these allocations to one may solve this problem. The worst case in dma_alloc_coherent happens when jumbo packet size = 4096, previous code needs 64K in one allocation at most, this change needs 85K in one allocation. When jumbo packet size 2048, previous code needs 12K in one allocation at most, this change needs 29K. When jumbo packet size is between 2048 and 4096, previous code needs 18K in one allocation at most, this change needs 48K. Hope my calculation is right. I'm not sure if it's acceptable. Also, I think a better way to do this allocation would be to create a single struct with the items to be allocated, where as few as possible of the struct members are of dynamic size (drivers/net/ethernet/cirrus/ep93xx_eth.c is a nice example of this) - although this reason alone would not be enough to reject the alloc patches, but the previous reason might. This way is actually the better way, but it needs lots of changes to the original code. Maybe I would try it later. Cheers, Mark ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: speakup: synth.c: removed a space
On Wed, 2013-12-04 at 10:38 +0300, Dan Carpenter wrote:
On Wed, Dec 04, 2013 at 06:35:15AM +0200, Aldo Iljazi wrote:
Samuel Thibault wrote:
Err, I'd rather make it really visible that the for loop doesn't have
its first statement?
Wouldn't it be better if you add a comment there? So it would follow the
coding style?
No. Adding obvious comments is more annoying than the space.
This seems like a small bug in checkpatch.pl. Joe, the problem is this
code:
for ( ; x; x++) {
It's complaining about the space character before the semicolon.
Shrug. checkpatch isn't and can't be perfect.
I think it's for things like
for (;;)
and that's better than
for ( ; ; )
or
for ( ;; )
or
for ( ;;)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: bcm: DDRInit: Fixed non_standard kernel style.
On Wed, Dec 04, 2013 at 04:53:09AM -0500, Gary Rookard wrote:
Fixed non_standard kernel style.
You're doing too many things at once, it needs to be broken into
a patch series which do one type of change at a time.
The changelog is useless.
Signed off by: Gary Alan Rookard garyrook...@gmail.com
---
drivers/staging/bcm/DDRInit.c | 2081
+++--
1 file changed, 977 insertions(+), 1104 deletions(-)
diff --git a/drivers/staging/bcm/DDRInit.c b/drivers/staging/bcm/DDRInit.c
index 9f7e30f..2fc3ba9 100644
--- a/drivers/staging/bcm/DDRInit.c
+++ b/drivers/staging/bcm/DDRInit.c
@@ -1,977 +1,876 @@
-#include headers.h
-
+/* File: DDRInit.c */
This isn't useful.
+ {0x0F000840, 0x0FFF1B00}, /* Changed Source for X-bar and MIPS Clock
to APLL */
+ {0xF870, 0x0002},
+ {0x0F00A044, 0x1FFF},
+ {0x0F00A040, 0x1F00},
One of these is not like the others. You have introduced a bug with
0xF870 vs 0x0F000870.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 19/51] staging: comedi: me4000: refactor request_irq() during attach
On Wed, Dec 04, 2013 at 11:10:05AM +, Ian Abbott wrote:
On 2013-12-03 19:07, H Hartley Sweeten wrote:
Do the request_irq() before setting up the subdevices. This removes an
indent level and makes the code a bit cleaner.
Also, remove the dev_warn() noise about the irq.
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
drivers/staging/comedi/drivers/me4000.c | 29 +
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/comedi/drivers/me4000.c
b/drivers/staging/comedi/drivers/me4000.c
index 3d12e91..b7284d0 100644
--- a/drivers/staging/comedi/drivers/me4000.c
+++ b/drivers/staging/comedi/drivers/me4000.c
@@ -1505,6 +1505,13 @@ static int me4000_auto_attach(struct comedi_device
*dev,
me4000_reset(dev);
+if (pcidev-irq 0) {
+result = request_irq(pcidev-irq, me4000_ai_isr, IRQF_SHARED,
+ dev-board_name, dev);
+if (result == 0)
+dev-irq = pcidev-irq;
+}
+
result = comedi_alloc_subdevices(dev, 4);
This will cause a NULL pointer dereference on the following line in
me4000_ai_isr():
struct comedi_subdevice *s = dev-subdevices[0];
That's not a dereference, you're just taking the address. Even if dev
is NULL it won't crash.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/11] staging: et131x: simplify rx dma code
On Wed, Dec 4, 2013 at 8:01 PM, ZHAO Gang gamer...@gmail.com wrote: On Wed, Dec 4, 2013 at 5:05 PM, Mark Einon mark.ei...@gmail.com wrote: On Wed, Dec 04, 2013 at 03:24:18PM +0800, ZHAO Gang wrote: The original code allocate rx dma memory in several dma_alloc_coherent calls, which causes some problems: 1. since dma_alloc_coherent allocate at least one page memory, it wastes some memory when allocation size is smaller than one page. 2. it causes et131x_rx_dma_memory_free as complex as et131x_rx_dma_memory_alloc Instead, allocate all rx dma memory in one dma_alloc_coherent call makes less code, makes it easy to handle dma allocation error, and makes et131x_rx_dma_memory_free as simple as it could be. Signed-off-by: ZHAO Gang gamer...@gmail.com --- drivers/staging/et131x/et131x.c | 202 1 file changed, 59 insertions(+), 143 deletions(-) Hi Zhao, I haven't had time to look at these last few patches in detail yet, but I do have some comments - The code is an improvement in that it reduces the number of lines of code, but you're also making the driver more susceptible to memory allocation issues, as you're asking for a single large block of memory and not several smaller ones. I'm not sure if that will have an appreciable impact yet. The first motivation that I want to change the dma alloc code is to add error check code, then I find it's not trivial to add it, since there are several dma_alloc_coherent, some in for loop, if dma alloc failed in the middle, code to free already allocated dma memery will be hard to read, so I think combine these allocations to one may solve this problem. The worst case in dma_alloc_coherent happens when jumbo packet size = 4096, previous code needs 64K in one allocation at most, this change needs 85K in one allocation. When jumbo packet size 2048, previous code needs 12K in one allocation at most, this change needs 29K. When jumbo packet size is between 2048 and 4096, previous code needs 18K in one allocation at most, this change needs 48K. Hope my calculation is right. I'm not sure if it's acceptable. By re-examine the code I found the calculation is not correct. The real impact is too big to apply this patch, so this patch and following patches should be dropped. Also, I think a better way to do this allocation would be to create a single struct with the items to be allocated, where as few as possible of the struct members are of dynamic size (drivers/net/ethernet/cirrus/ep93xx_eth.c is a nice example of this) - although this reason alone would not be enough to reject the alloc patches, but the previous reason might. This way is actually the better way, but it needs lots of changes to the original code. Maybe I would try it later. Cheers, Mark ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 19/51] staging: comedi: me4000: refactor request_irq() during attach
On 2013/12/04 01:05 PM, Dan Carpenter wrote:
On Wed, Dec 04, 2013 at 11:10:05AM +, Ian Abbott wrote:
On 2013-12-03 19:07, H Hartley Sweeten wrote:
Do the request_irq() before setting up the subdevices. This removes an
indent level and makes the code a bit cleaner.
Also, remove the dev_warn() noise about the irq.
Signed-off-by: H Hartley Sweeten hswee...@visionengravers.com
Cc: Ian Abbott abbo...@mev.co.uk
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
---
drivers/staging/comedi/drivers/me4000.c | 29 +
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/comedi/drivers/me4000.c
b/drivers/staging/comedi/drivers/me4000.c
index 3d12e91..b7284d0 100644
--- a/drivers/staging/comedi/drivers/me4000.c
+++ b/drivers/staging/comedi/drivers/me4000.c
@@ -1505,6 +1505,13 @@ static int me4000_auto_attach(struct comedi_device
*dev,
me4000_reset(dev);
+ if (pcidev-irq 0) {
+ result = request_irq(pcidev-irq, me4000_ai_isr, IRQF_SHARED,
+ dev-board_name, dev);
+ if (result == 0)
+ dev-irq = pcidev-irq;
+ }
+
result = comedi_alloc_subdevices(dev, 4);
This will cause a NULL pointer dereference on the following line in
me4000_ai_isr():
struct comedi_subdevice *s = dev-subdevices[0];
That's not a dereference, you're just taking the address. Even if dev
is NULL it won't crash.
Yes, I'll give you that one!
--
-=( Ian Abbott @ MEV Ltd.E-mail: abbo...@mev.co.uk )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 4/9] staging: android: binder: Add align_helper() macro
This patch adds align_helper() macro that will be used for enforcing
the desired alignment on 64bit systems where the alignment will differ
depending on the userspace used (32bit /64bit).
This patch is a temporary patch that will be extended with 32bit compat
handling.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c |9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 16109cd..6719a53 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -140,6 +140,8 @@ module_param_call(stop_on_user_error,
binder_set_stop_on_user_error,
binder_stop_on_user_error = 2; \
} while (0)
+#define align_helper(ptr) ALIGN(ptr, sizeof(void *))
+
enum binder_stat_types {
BINDER_STAT_PROC,
BINDER_STAT_THREAD,
@@ -1239,7 +1241,7 @@ static void binder_transaction_buffer_release(struct
binder_proc *proc,
if (buffer-target_node)
binder_dec_node(buffer-target_node, 1, 0);
- offp = (size_t *)(buffer-data + ALIGN(buffer-data_size, sizeof(void
*)));
+ offp = (size_t *)(buffer-data + align_helper(buffer-data_size));
if (failed_at)
off_end = failed_at;
else
@@ -1472,7 +1474,7 @@ static void binder_transaction(struct binder_proc *proc,
if (target_node)
binder_inc_node(target_node, 1, 0, NULL);
- offp = (size_t *)(t-buffer-data + ALIGN(tr-data_size, sizeof(void
*)));
+ offp = (size_t *)(t-buffer-data + align_helper(tr-data_size));
if (copy_from_user(t-buffer-data, tr-data.ptr.buffer,
tr-data_size)) {
binder_user_error(%d:%d got transaction with invalid data
ptr\n,
@@ -2378,8 +2380,7 @@ retry:
tr.data.ptr.buffer = (void *)t-buffer-data +
proc-user_buffer_offset;
tr.data.ptr.offsets = tr.data.ptr.buffer +
- ALIGN(t-buffer-data_size,
- sizeof(void *));
+ align_helper(t-buffer-data_size);
if (binder_copy_to_user(cmd, tr, ptr, sizeof(struct
binder_transaction_data)))
return -EFAULT;
--
1.7.9.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 6/9] staging: android: binder: Add size_helper() macro
This patch adds size_helper() macro that will be used for indexing into
different buffers on 64bit systems where the size of particular
structures will differ depending on the userspace used (32bit /64bit).
This patch is a temporary patch that will be extended with 32bit compat
handling.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 95c2581..6d22717 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -142,6 +142,7 @@ module_param_call(stop_on_user_error,
binder_set_stop_on_user_error,
#define align_helper(ptr) ALIGN(ptr, sizeof(void *))
#define deref_helper(ptr) (*(typeof(size_t *))ptr)
+#define size_helper(x) sizeof(x)
enum binder_stat_types {
BINDER_STAT_PROC,
@@ -1247,10 +1248,10 @@ static void binder_transaction_buffer_release(struct
binder_proc *proc,
off_end = failed_at;
else
off_end = (void *)offp + buffer-offsets_size;
- for (; offp off_end; offp += sizeof(size_t)) {
+ for (; offp off_end; offp += size_helper(size_t)) {
struct flat_binder_object *fp;
- if (deref_helper(offp) buffer-data_size - sizeof(*fp) ||
- buffer-data_size sizeof(*fp) ||
+ if (deref_helper(offp) buffer-data_size - size_helper(*fp) ||
+ buffer-data_size size_helper(*fp) ||
!IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
pr_err(transaction release %d bad offset %zd, size
%zd\n,
debug_id, deref_helper(offp), buffer-data_size);
@@ -1489,17 +1490,17 @@ static void binder_transaction(struct binder_proc *proc,
return_error = BR_FAILED_REPLY;
goto err_copy_data_failed;
}
- if (!IS_ALIGNED(tr-offsets_size, sizeof(size_t))) {
+ if (!IS_ALIGNED(tr-offsets_size, size_helper(size_t))) {
binder_user_error(%d:%d got transaction with invalid offsets
size, %zd\n,
proc-pid, thread-pid, tr-offsets_size);
return_error = BR_FAILED_REPLY;
goto err_bad_offset;
}
off_end = (void *)offp + tr-offsets_size;
- for (; offp off_end; offp += sizeof(size_t)) {
+ for (; offp off_end; offp += size_helper(size_t)) {
struct flat_binder_object *fp;
- if (deref_helper(offp) t-buffer-data_size - sizeof(*fp) ||
- t-buffer-data_size sizeof(*fp) ||
+ if (deref_helper(offp) t-buffer-data_size -
size_helper(*fp) ||
+ t-buffer-data_size size_helper(*fp) ||
!IS_ALIGNED(deref_helper(offp), sizeof(u32))) {
binder_user_error(%d:%d got transaction with invalid
offset, %zd\n,
proc-pid, thread-pid,
deref_helper(offp));
@@ -2229,7 +2230,7 @@ retry:
break;
}
- if (end - ptr sizeof(tr) + 4)
+ if (end - ptr size_helper(tr) + 4)
break;
switch (w-type) {
--
1.7.9.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()
This patch adds binder_copy_to_user() to be used for copying binder
commands to user address space. This way we can abstract away the
copy_to_user() calls and add separate handling for the compat layer.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c | 39 --
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 233889c..6fbb340 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2117,6 +2117,18 @@ static int binder_has_thread_work(struct binder_thread
*thread)
(thread-looper BINDER_LOOPER_STATE_NEED_RETURN);
}
+static int binder_copy_to_user(uint32_t cmd, void *parcel,
+ void __user **ptr, size_t size)
+{
+ if (put_user(cmd, (uint32_t __user *)*ptr))
+ return -EFAULT;
+ *ptr += sizeof(uint32_t);
+ if (copy_to_user(*ptr, parcel, size))
+ return -EFAULT;
+ *ptr += size;
+ return 0;
+}
+
static int binder_thread_read(struct binder_proc *proc,
struct binder_thread *thread,
void __user *buffer, size_t size,
@@ -2263,15 +2275,12 @@ retry:
node-has_weak_ref = 0;
}
if (cmd != BR_NOOP) {
- if (put_user(cmd, (uint32_t __user *)ptr))
- return -EFAULT;
- ptr += sizeof(uint32_t);
- if (put_user(node-ptr, (void * __user *)ptr))
- return -EFAULT;
- ptr += sizeof(void *);
- if (put_user(node-cookie, (void * __user
*)ptr))
+ struct binder_ptr_cookie tmp;
+
+ tmp.ptr = node-ptr;
+ tmp.cookie = node-cookie;
+ if (binder_copy_to_user(cmd, tmp, ptr,
sizeof(struct binder_ptr_cookie)))
return -EFAULT;
- ptr += sizeof(void *);
binder_stat_br(proc, thread, cmd);
binder_debug(BINDER_DEBUG_USER_REFS,
@@ -2306,12 +2315,10 @@ retry:
cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
else
cmd = BR_DEAD_BINDER;
- if (put_user(cmd, (uint32_t __user *)ptr))
- return -EFAULT;
- ptr += sizeof(uint32_t);
- if (put_user(death-cookie, (void * __user *)ptr))
+
+ if (binder_copy_to_user(cmd, death-cookie, ptr,
sizeof(void *)))
return -EFAULT;
- ptr += sizeof(void *);
+
binder_stat_br(proc, thread, cmd);
binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
%d:%d %s %p\n,
@@ -2373,12 +2380,8 @@ retry:
ALIGN(t-buffer-data_size,
sizeof(void *));
- if (put_user(cmd, (uint32_t __user *)ptr))
- return -EFAULT;
- ptr += sizeof(uint32_t);
- if (copy_to_user(ptr, tr, sizeof(tr)))
+ if (binder_copy_to_user(cmd, tr, ptr, sizeof(struct
binder_transaction_data)))
return -EFAULT;
- ptr += sizeof(tr);
trace_binder_transaction_received(t);
binder_stat_br(proc, thread, cmd);
--
1.7.9.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 7/9] staging: android: binder: Add copy_flat_binder_object()
This patch adds copy_flat_binder_object macro() that will help
dereference struct flat_binder_object on 64bit systems where the
structure differs between 32bit and 64bit userspace.
This patch is a temporary patch that will be extended with 32bit compat
handling.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c |9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index 6d22717..855d348 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -144,6 +144,11 @@ module_param_call(stop_on_user_error,
binder_set_stop_on_user_error,
#define deref_helper(ptr) (*(typeof(size_t *))ptr)
#define size_helper(x) sizeof(x)
+static inline struct flat_binder_object *copy_flat_binder_object(void __user
*ptr)
+{
+ return (struct flat_binder_object *)ptr;
+}
+
enum binder_stat_types {
BINDER_STAT_PROC,
BINDER_STAT_THREAD,
@@ -1257,7 +1262,7 @@ static void binder_transaction_buffer_release(struct
binder_proc *proc,
debug_id, deref_helper(offp), buffer-data_size);
continue;
}
- fp = (struct flat_binder_object *)(buffer-data +
deref_helper(offp));
+ fp = copy_flat_binder_object(buffer-data + deref_helper(offp));
switch (fp-type) {
case BINDER_TYPE_BINDER:
case BINDER_TYPE_WEAK_BINDER: {
@@ -1507,7 +1512,7 @@ static void binder_transaction(struct binder_proc *proc,
return_error = BR_FAILED_REPLY;
goto err_bad_offset;
}
- fp = (struct flat_binder_object *)(t-buffer-data +
deref_helper(offp));
+ fp = copy_flat_binder_object(t-buffer-data +
deref_helper(offp));
switch (fp-type) {
case BINDER_TYPE_BINDER:
case BINDER_TYPE_WEAK_BINDER: {
--
1.7.9.5
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 8/9] staging: android: binder: Add binder compat handling to binder.h
This patch adds all the needed compat structures to binder.h. All the
structures defined in this patch mirror the structure and size of 32bit
ones.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.h | 109 ++
1 file changed, 109 insertions(+)
diff --git a/drivers/staging/android/binder.h b/drivers/staging/android/binder.h
index cbe3451..6c9849d 100644
--- a/drivers/staging/android/binder.h
+++ b/drivers/staging/android/binder.h
@@ -22,6 +22,10 @@
#include linux/ioctl.h
+#ifdef CONFIG_COMPAT
+#include linux/compat.h
+#endif
+
#define B_PACK_CHARS(c1, c2, c3, c4) \
c1)24)) | (((c2)16)) | (((c3)8)) | (c4))
#define B_TYPE_LARGE 0x85
@@ -326,5 +330,110 @@ enum binder_driver_command_protocol {
*/
};
+/* Support for 32bit userspace on a 64bit system */
+#ifdef CONFIG_COMPAT
+struct compat_flat_binder_object {
+ /* 8 bytes for large_flat_header. */
+ __u32 type;
+ __u32 flags;
+
+ /* 8 bytes of data. */
+ union {
+ compat_uptr_t binder; /* local object */
+ __u32 handle; /* remote object */
+ };
+
+ /* extra data associated with local object */
+ compat_uptr_t cookie;
+};
+
+struct compat_binder_write_read {
+ compat_size_t write_size; /* bytes to write */
+ compat_size_t write_consumed; /* bytes consumed by driver */
+ compat_ulong_t write_buffer;
+ compat_size_t read_size; /* bytes to read */
+ compat_size_t read_consumed; /* bytes consumed by driver */
+ compat_ulong_t read_buffer;
+};
+
+#define COMPAT_BINDER_WRITE_READ _IOWR('b', 1, struct
compat_binder_write_read)
+
+struct compat_binder_transaction_data {
+ /* The first two are only used for bcTRANSACTION and brTRANSACTION,
+* identifying the target and contents of the transaction.
+*/
+ union {
+ __u32 handle; /* target descriptor of command
transaction */
+ compat_uptr_t ptr;/* target descriptor of return
transaction */
+ } target;
+ compat_uptr_t cookie; /* target object cookie */
+ __u32 code; /* transaction command */
+
+ /* General information about the transaction. */
+ __u32 flags;
+ pid_t sender_pid;
+ uid_t sender_euid;
+ compat_size_t data_size; /* number of bytes of data */
+ compat_size_t offsets_size; /* number of bytes of offsets */
+
+ /* If this transaction is inline, the data immediately
+* follows here; otherwise, it ends with a pointer to
+* the data buffer.
+*/
+ union {
+ struct {
+ /* transaction data */
+ compat_uptr_t buffer;
+ /* offsets from buffer to flat_binder_object structs */
+ compat_uptr_t offsets;
+ } ptr;
+ __u8buf[8];
+ } data;
+};
+
+struct compat_binder_ptr_cookie {
+ compat_uptr_t ptr;
+ compat_uptr_t cookie;
+};
+
+/* legacy - not used anymore */
+struct compat_binder_pri_ptr_cookie {
+ __s32 priority;
+ compat_uptr_t ptr;
+ compat_uptr_t cookie;
+};
+
+enum compat_binder_driver_return_protocol {
+ COMPAT_BR_TRANSACTION = _IOR('r', 2, struct
compat_binder_transaction_data),
+ COMPAT_BR_REPLY = _IOR('r', 3, struct compat_binder_transaction_data),
+
+ COMPAT_BR_INCREFS = _IOR('r', 7, struct compat_binder_ptr_cookie),
+ COMPAT_BR_ACQUIRE = _IOR('r', 8, struct compat_binder_ptr_cookie),
+ COMPAT_BR_RELEASE = _IOR('r', 9, struct compat_binder_ptr_cookie),
+ COMPAT_BR_DECREFS = _IOR('r', 10, struct compat_binder_ptr_cookie),
+
+ /* legacy - not used anymore */
+ COMPAT_BR_ATTEMPT_ACQUIRE = _IOR('r', 11, struct
compat_binder_pri_ptr_cookie),
+
+ COMPAT_BR_DEAD_BINDER = _IOR('r', 15, compat_uptr_t),
+ COMPAT_BR_CLEAR_DEATH_NOTIFICATION_DONE = _IOR('r', 16, compat_uptr_t),
+};
+
+enum compat_binder_driver_command_protocol {
+ COMPAT_BC_TRANSACTION = _IOW('c', 0, struct
compat_binder_transaction_data),
+ COMPAT_BC_REPLY = _IOW('c', 1, struct compat_binder_transaction_data),
+
+ COMPAT_BC_FREE_BUFFER = _IOW('c', 3, compat_uptr_t),
+
+ COMPAT_BC_INCREFS_DONE = _IOW('c', 8, struct compat_binder_ptr_cookie),
+ COMPAT_BC_ACQUIRE_DONE = _IOW('c', 9, struct compat_binder_ptr_cookie),
+
+ COMPAT_BC_REQUEST_DEATH_NOTIFICATION = _IOW('c', 14, struct
compat_binder_ptr_cookie),
+ COMPAT_BC_CLEAR_DEATH_NOTIFICATION = _IOW('c', 15, struct
compat_binder_ptr_cookie),
+
+ COMPAT_BC_DEAD_BINDER_DONE = _IOW('c', 16, compat_uptr_t),
+};
+#endif /* CONFIG_COMPAT */
+
#endif /* _LINUX_BINDER_H */
--
1.7.9.5
___
devel
[PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction
This patch moves some of the logic for binder_thread_write() into
subfunctions. This way we can share more code with the binder compat
layer.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c | 403 +-
1 file changed, 220 insertions(+), 183 deletions(-)
diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c
index eaec1da..233889c 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -1700,6 +1700,217 @@ err_no_context_mgr_node:
thread-return_error = return_error;
}
+static void bc_increfs_done(struct binder_proc *proc,
+ struct binder_thread *thread, uint32_t cmd,
+ void __user *node_ptr, void __user *cookie)
+{
+ struct binder_node *node;
+
+ node = binder_get_node(proc, node_ptr);
+ if (node == NULL) {
+ binder_user_error(%d:%d %s u%p no match\n,
+ proc-pid, thread-pid,
+ cmd == BC_INCREFS_DONE ?
+ BC_INCREFS_DONE :
+ BC_ACQUIRE_DONE,
+ node_ptr);
+ return;
+ }
+ if (cookie != node-cookie) {
+ binder_user_error(%d:%d %s u%p node %d cookie mismatch %p !=
%p\n,
+ proc-pid, thread-pid,
+ cmd == BC_INCREFS_DONE ?
+ BC_INCREFS_DONE : BC_ACQUIRE_DONE,
+ node_ptr, node-debug_id,
+ cookie, node-cookie);
+ return;
+ }
+ if (cmd == BC_ACQUIRE_DONE) {
+ if (node-pending_strong_ref == 0) {
+ binder_user_error(%d:%d BC_ACQUIRE_DONE node %d has no
pending acquire request\n,
+ proc-pid, thread-pid,
+ node-debug_id);
+ return;
+ }
+ node-pending_strong_ref = 0;
+ } else {
+ if (node-pending_weak_ref == 0) {
+ binder_user_error(%d:%d BC_INCREFS_DONE node %d has no
pending increfs request\n,
+ proc-pid, thread-pid,
+ node-debug_id);
+ return;
+ }
+ node-pending_weak_ref = 0;
+ }
+ binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
+ binder_debug(BINDER_DEBUG_USER_REFS,
+%d:%d %s node %d ls %d lw %d\n,
+proc-pid, thread-pid,
+cmd == BC_INCREFS_DONE ?
+BC_INCREFS_DONE :
+BC_ACQUIRE_DONE,
+node-debug_id, node-local_strong_refs,
+node-local_weak_refs);
+ return;
+}
+
+static void bc_free_buffer(struct binder_proc *proc,
+ struct binder_thread *thread, void __user *data_ptr)
+{
+ struct binder_buffer *buffer;
+
+ buffer = binder_buffer_lookup(proc, data_ptr);
+ if (buffer == NULL) {
+ binder_user_error(%d:%d BC_FREE_BUFFER u%p no match\n,
+ proc-pid, thread-pid, data_ptr);
+ return;
+ }
+ if (!buffer-allow_user_free) {
+ binder_user_error(%d:%d BC_FREE_BUFFER u%p matched unreturned
buffer\n,
+ proc-pid, thread-pid, data_ptr);
+ return;
+ }
+ binder_debug(BINDER_DEBUG_FREE_BUFFER,
+%d:%d BC_FREE_BUFFER u%p found buffer %d for %s
transaction\n,
+proc-pid, thread-pid, data_ptr, buffer-debug_id,
+buffer-transaction ? active : finished);
+
+ if (buffer-transaction) {
+ buffer-transaction-buffer = NULL;
+ buffer-transaction = NULL;
+ }
+ if (buffer-async_transaction buffer-target_node) {
+ BUG_ON(!buffer-target_node-has_async_transaction);
+ if (list_empty(buffer-target_node-async_todo))
+ buffer-target_node-has_async_transaction = 0;
+ else
+ list_move_tail(buffer-target_node-async_todo.next,
thread-todo);
+ }
+ trace_binder_transaction_buffer_release(buffer);
+ binder_transaction_buffer_release(proc, buffer, NULL);
+ binder_free_buf(proc, buffer);
+ return;
+}
+
+static void bc_clear_death_notif(struct binder_proc *proc,
+ struct binder_thread *thread, uint32_t cmd,
+ uint32_t target, void __user *cookie)
+{
+ struct binder_ref *ref;
+ struct binder_ref_death *death;
+
+ ref = binder_get_ref(proc, target);
+ if (ref == NULL) {
+ binder_user_error(%d:%d %s invalid ref %d\n,
+ proc-pid, thread-pid,
+ cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+
[PATCH -next] checkpatch: Warn only on space before semicolon at end of line
The space before a non-naked semicolon test has
unwanted output when used in for ( ;; ) loops.
Make the test work only on end-of-line statement
termination semicolons.
Signed-off-by: Joe Perches j...@perches.com
---
On Wed, 2013-12-04 at 11:21 +0300, Dan Carpenter wrote:
You and I generally agree on style preferences...
True, and I think that's a good thing.
I think the warning
should be limited to grep ;$.
Look at the output of:
$ git grep ; -- *.[ch] | grep -w for
...
The | wc -l output above in -next is
1407
Which of those should not be warned on?
$ git grep ; -- *.[ch] | grep -P \bfor\s*\(\s+; | wc -l
211
I think all of the uses like for ( ; expression; expression)
are unbalanced and should be avoided.
I go off and look at checkpatch output
Oh. checkpatch doesn't complain about spacing around
semicolon with for loops here. This error is a separate
test that looks only for space before semicolon.
So, we agree after all.
This test should look only for space before end-of-statement
semicolons at EOL.
This has still has false positives with multi-line fors like:
for ( ; expression ;
expression);
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 38be5d5..d26eac6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3131,7 +3131,7 @@ sub process {
}
# check for whitespace before a non-naked semicolon
- if ($line =~ /^\+.*\S\s+;/) {
+ if ($line =~ /^\+.*\S\s+;\s*$/) {
if (WARN(SPACING,
space prohibited before semicolon\n .
$herecurr)
$fix) {
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 04, 2013 at 06:09:41PM +, Serban Constantinescu wrote:
+#define size_helper(x) ({\
+ size_t __size; \
+ if (!is_compat_task()) \
+ __size = sizeof(x); \
+ else if (sizeof(x) == sizeof(struct flat_binder_object))\
+ __size = sizeof(struct compat_flat_binder_object); \
+ else if (sizeof(x) == sizeof(struct binder_transaction_data)) \
+ __size = sizeof(struct compat_binder_transaction_data); \
+ else if (sizeof(x) == sizeof(size_t)) \
+ __size = sizeof(compat_size_t); \
+ else\
+ BUG(); \
+ __size; \
+ })
Ick.
First off, no driver should ever be able to crash the kernel, which you
just did.
Second, almost none of those if lines will ever be hit, why did you
include it all?
And finally, is this all really needed? Why not just fix the structures
to be correct, and then fix userspace to use the correct structures as
well, thereby not needing a compat layer at all?
You have the chance to fix the api properly, why not take it and do it,
making all of this unnecessary.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 1:43 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. I would suggest fixing the 32-bit structures to use fixed sizes where appropriate (__u32 instead of unsigned long) while maintaining compatibility, and then using compat_ioctl where necessary to handle pointers. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 1:43 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. Not if you make the cast right at the beginning, when you first touch the data, but yes, it does take some of the type saftey away, at the expense of simpler code to mess up :) That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. Wait, I thought that libbinder would have to be changed anyway here, to handle 64bit kernels (in both 32 and 64bit userspace). Since you are already changing it, why not just do it correctly? Or does this patch series mean that no userspace code is changed? Is that a requirement here? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 2:02 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 1:43 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. Not if you make the cast right at the beginning, when you first touch the data, but yes, it does take some of the type saftey away, at the expense of simpler code to mess up :) That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. Wait, I thought that libbinder would have to be changed anyway here, to handle 64bit kernels (in both 32 and 64bit userspace). Since you are already changing it, why not just do it correctly? libbinder will need changes to support 64-bit userspace and especially a mixed 64-bit and 32-bit userspace, but this patch series is only addressing a pure 32-bit userspace on a 64-bit kernel. Support for a 64-bit userspace in Android is obviously going to require a future version of Android including, among other things, libbinder changes. As far as I know, those changes won't need to change the ioctl api, only the layout of the buffers that are passed through the ioctl api. Or does this patch series mean that no userspace code is changed? Is that a requirement here? Since this series only addresses 32-bit userspace on 64-bit kernel support there are no associated userspace changes. Changing the 32-bit api here means that combining the KitKat branch from http://android.googlesource.com with a newer kernel version will not work. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/51] staging: comedi: cleanup irq requests
On Wed, Dec 04, 2013 at 11:46:48AM +, Ian Abbott wrote: On 2013-12-03 19:07, H Hartley Sweeten wrote: The comedi subsystem only requires the drivers to support interrupts if one or more of the subdevices support async commands. Since this is optional: 1) don't fail the attach if the irq is not available 2) only hookup the async command support if the irq is available 3) remove any async command init from subdevices that don't need it 4) remove any unnecessary sanity checks in the async command functions Make use of the comedi_device 'read_subdev' and 'write_subdev' pointers instead of accessing the dev-subdevices array directly with magic numbers to the correct subdevice. Also, remove any unnecessary debug noise associated with the irq requests. H Hartley Sweeten (51): staging: comedi: s626: fix async command hookup staging: comedi: pcl816: remove 'irq_free' from private data staging: comedi: pcl816: only init command support if irq is available staging: comedi: pcl816: remove 'sub_ai' from private data staging: comedi: pcl816: use dev-read_subdev staging: comedi: pcl818: remove 'irq_free' from private data staging: comedi: pcl818: remove unnecessary 'dev-irq' tests staging: comedi: pcl818: remove function trace noise staging: comedi: pcl818: only init async command members when needed staging: comedi: pcl818: remove 'sub_ai' from private data staging: comedi: pcl818: use dev-read_subdev staging: comedi: pcl818: remove unnecessary s-len_chanlist init staging: comedi: pcl812: remove unnecessary s-len_chanlist init staging: comedi: pcl812: only init async command members when needed staging: comedi: pcl812: use dev-read_subdev staging: comedi: ni_pcimio: tidy up the irq request staging: comedi: ni_pcidio: tidy up the irq request staging: comedi: ni_at_a2150: tidy up the irq request staging: comedi: me4000: refactor request_irq() during attach staging: comedi: me4000: use dev-read_subdev staging: comedi: me4000: remove unnecessary check in the irq handler staging: comedi: das1800: tidy up irq request staging: comedi: das1800: only init command support if irq is available staging: comedi: das1800: remove unnecessary 'dev-irq' test staging: comedi: das1800: use dev-read_subdev staging: comedi: das16m1: remove unnecessary 'dev-irq' test staging: comedi: adl_pci9111: fix incorrect irq passed to request_irq() staging: comedi: adl_pci9111: the irq is only needed for async command support staging: comedi: adl_pci9111: remove unnecessary 'dev-irq' test staging: comedi: dt2814: use dev-read_subdev staging: comedi: dt282x: use dev-read_subdev staging: comedi: dt282x: use dev-write_subdev staging: comedi: amplc_pci230: tidy up irq request staging: comedi: adl_pci9118: tidy up irq request staging: comedi: adv_pci1710: only init async command members when needed staging: comedi: adv_pci1710: use dev-read_subdev staging: comedi: dt3000: don't fail attach if irq is not available staging: comedi: dt3000: use dev-read_subdev staging: comedi: s626: use dev-read_subdev staging: comedi: hwrdv_apci3120: use dev-read_subdev staging: comedi: hwrdv_apci3200: use dev-read_subdev staging: comedi: adl_pci9118: use dev-read_subdev staging: comedi: amplc_pc236: use dev-read_subdev staging: comedi: amplc_pci224: use dev-write_subdev staging: comedi: ni_65xx: use dev-read_subdev staging: comedi: ni_atmio16d: use dev-read_subdev staging: comedi: rtd520: use dev-read_subdev staging: comedi: ni_pcidio: factor board reset out of attach staging: comedi: ni_pcidio: request_irq() before seting up subdevices staging: comedi: ni_pcidio: use dev-read_subdev staging: comedi: multiq3: pass subdevice to encoder_reset() .../comedi/drivers/addi-data/hwdrv_apci3120.c | 6 +- .../comedi/drivers/addi-data/hwdrv_apci3200.c | 2 +- drivers/staging/comedi/drivers/adl_pci9111.c | 31 drivers/staging/comedi/drivers/adl_pci9118.c | 45 +-- drivers/staging/comedi/drivers/adv_pci1710.c | 10 +-- drivers/staging/comedi/drivers/amplc_pc236.c | 2 +- drivers/staging/comedi/drivers/amplc_pci224.c | 2 +- drivers/staging/comedi/drivers/amplc_pci230.c | 27 +++ drivers/staging/comedi/drivers/das16m1.c | 5 -- drivers/staging/comedi/drivers/das1800.c | 89 +- drivers/staging/comedi/drivers/dt2814.c| 4 +- drivers/staging/comedi/drivers/dt282x.c| 10 +-- drivers/staging/comedi/drivers/dt3000.c| 29 +++ drivers/staging/comedi/drivers/me4000.c| 37 - drivers/staging/comedi/drivers/multiq3.c | 6 +- drivers/staging/comedi/drivers/ni_65xx.c | 2
Re: [PATCH 04/11] staging: et131x: drop packet when error occurs in et131x_tx
On Wed, Dec 04, 2013 at 03:24:14PM +0800, ZHAO Gang wrote: As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY when tx failed. Really? That's ok to do? Seems like you are changing the logic of the function a lot here, how does the code let userspace know packets were dropped then? greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 2/9] staging: android: binder: Add binder_copy_to_user()
On Wed, Dec 04, 2013 at 06:09:34PM +, Serban Constantinescu wrote:
This patch adds binder_copy_to_user() to be used for copying binder
commands to user address space. This way we can abstract away the
copy_to_user() calls and add separate handling for the compat layer.
Signed-off-by: Serban Constantinescu serban.constantine...@arm.com
---
drivers/staging/android/binder.c | 39
--
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/android/binder.c
b/drivers/staging/android/binder.c
index 233889c..6fbb340 100644
--- a/drivers/staging/android/binder.c
+++ b/drivers/staging/android/binder.c
@@ -2117,6 +2117,18 @@ static int binder_has_thread_work(struct binder_thread
*thread)
(thread-looper BINDER_LOOPER_STATE_NEED_RETURN);
}
+static int binder_copy_to_user(uint32_t cmd, void *parcel,
+void __user **ptr, size_t size)
+{
+ if (put_user(cmd, (uint32_t __user *)*ptr))
+ return -EFAULT;
+ *ptr += sizeof(uint32_t);
+ if (copy_to_user(*ptr, parcel, size))
+ return -EFAULT;
+ *ptr += size;
+ return 0;
+}
I know what you are trying to do here, but ick, why not just use the
structure involved in the copying out here? Or just copy the thing out
in one chunk, not two different calls, which should make this go
faster, right?
+
static int binder_thread_read(struct binder_proc *proc,
struct binder_thread *thread,
void __user *buffer, size_t size,
@@ -2263,15 +2275,12 @@ retry:
node-has_weak_ref = 0;
}
if (cmd != BR_NOOP) {
- if (put_user(cmd, (uint32_t __user *)ptr))
- return -EFAULT;
- ptr += sizeof(uint32_t);
- if (put_user(node-ptr, (void * __user *)ptr))
- return -EFAULT;
- ptr += sizeof(void *);
- if (put_user(node-cookie, (void * __user
*)ptr))
+ struct binder_ptr_cookie tmp;
+
+ tmp.ptr = node-ptr;
+ tmp.cookie = node-cookie;
+ if (binder_copy_to_user(cmd, tmp, ptr,
sizeof(struct binder_ptr_cookie)))
return -EFAULT;
- ptr += sizeof(void *);
Are you sure this is correct? You are now no longer incrementing ptr
anymore, is that ok with the larger loop here?
binder_stat_br(proc, thread, cmd);
binder_debug(BINDER_DEBUG_USER_REFS,
@@ -2306,12 +2315,10 @@ retry:
cmd = BR_CLEAR_DEATH_NOTIFICATION_DONE;
else
cmd = BR_DEAD_BINDER;
- if (put_user(cmd, (uint32_t __user *)ptr))
- return -EFAULT;
- ptr += sizeof(uint32_t);
- if (put_user(death-cookie, (void * __user *)ptr))
+
+ if (binder_copy_to_user(cmd, death-cookie, ptr,
sizeof(void *)))
return -EFAULT;
- ptr += sizeof(void *);
+
Same here, no more ptr incrementing.
binder_stat_br(proc, thread, cmd);
binder_debug(BINDER_DEBUG_DEATH_NOTIFICATION,
%d:%d %s %p\n,
@@ -2373,12 +2380,8 @@ retry:
ALIGN(t-buffer-data_size,
sizeof(void *));
- if (put_user(cmd, (uint32_t __user *)ptr))
- return -EFAULT;
- ptr += sizeof(uint32_t);
- if (copy_to_user(ptr, tr, sizeof(tr)))
+ if (binder_copy_to_user(cmd, tr, ptr, sizeof(struct
binder_transaction_data)))
return -EFAULT;
- ptr += sizeof(tr);
And here, no more ptr incrementing.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 01/11] staging: et131x: make some tweaks to reduce split lines
On Wed, Dec 04, 2013 at 03:24:11PM +0800, ZHAO Gang wrote: 1. As TODO list suggested, do this sort of things to reduce split lines: struct fbr_lookup *fbr; fbr = rx_local-fbr[id]; Then replace all the instances of rx_local-fbr[id] with fbr. 2. Some code style change Please, only do ONE thing per patch, not two things, not three things, only 1. You are doing multiple things here, right? Please don't. Sorry, I can't take this patch, please fix it up and resend. greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, 4 Dec 2013 10:35:54 -0800
Greg KH gre...@linuxfoundation.org wrote:
On Wed, Dec 04, 2013 at 06:09:41PM +, Serban Constantinescu wrote:
+#define size_helper(x) ({ \
+ size_t __size; \
+ if (!is_compat_task()) \
+ __size = sizeof(x); \
+ else if (sizeof(x) == sizeof(struct flat_binder_object))\
+ __size = sizeof(struct compat_flat_binder_object); \
+ else if (sizeof(x) == sizeof(struct binder_transaction_data)) \
+ __size = sizeof(struct compat_binder_transaction_data); \
+ else if (sizeof(x) == sizeof(size_t)) \
+ __size = sizeof(compat_size_t); \
+ else\
+BUG(); \
+ __size; \
+ })
Ick.
First off, no driver should ever be able to crash the kernel, which you
just did.
And which would appear to mean that this code hasn't actually been
tested - at least not properly with error cases ?
You talk about type safety too but your code is already full of
put_user(node-ptr, (void * __user *)ptr))
The binder_copy_to_user thing is pretty confusingly named - it sounds
like a wrapper but is in fact a whole set of operations with two
different values and extra cookie structures and the like
Would something like
binder_put_userptr(mode-ptr, ptr)
perhaps be a shade easier to follow as a set of changes, and less clunky ?
And 3/9 you could have done a clean up .. instead of replacing endless
repeats of
- cmd == BC_INCREFS_DONE ?
- BC_INCREFS_DONE :
- BC_ACQUIRE_DONE,
+ acquire ?
+ BC_ACQUIRE_DONE :
+ BC_INCREFS_DONE,
couldn't you do that bit in just one place ?
Ditto
- cmd == BC_REQUEST_DEATH_NOTIFICATION ?
+ request ?
BC_REQUEST_DEATH_NOTIFICATION :
BC_CLEAR_DEATH_NOTIFICATION,
The _helper stuff with type and size magic also really obfuscates the
code horribly
+static inline struct flat_binder_object *copy_flat_binder_object(void
__user *ptr) +{
+ return (struct flat_binder_object *)ptr;
+}
What were you arguing about type safety again ?
While I'm tempted to answer and that children is what happens when you
don't take your interfaces mainstream and peer review them in the first
place I appreciate it won't help ;-)
I think I'd rather see the structures fixed up to be correct and properly
type stable for 64bit on a 64bit box including u64 user pointers.
For 32bit then yes you probably have to do something icky like
struct binder_foo64 {
}
struct binder_foo_compat {
}
#if 32bit
#define binder_foo binder_foo_compat
#else
#define binder_foo binder_foo64
#endif
but I do think it would make the rest of the code look less like a lesson
on pointer and GNU extension obfuscation and when 32bit finally gets
buried the uglies can be removed.
Alan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 4:02 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 02:22:13PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 2:02 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 1:43 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. Not if you make the cast right at the beginning, when you first touch the data, but yes, it does take some of the type saftey away, at the expense of simpler code to mess up :) That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. Wait, I thought that libbinder would have to be changed anyway here, to handle 64bit kernels (in both 32 and 64bit userspace). Since you are already changing it, why not just do it correctly? libbinder will need changes to support 64-bit userspace and especially a mixed 64-bit and 32-bit userspace, but this patch series is only addressing a pure 32-bit userspace on a 64-bit kernel. Support for a 64-bit userspace in Android is obviously going to require a future version of Android including, among other things, libbinder changes. As far as I know, those changes won't need to change the ioctl api, only the layout of the buffers that are passed through the ioctl api. only means you can rearrange things at that point in time, as you will have to be doing that anyway :) Or does this patch series mean that no userspace code is changed? Is that a requirement here? Since this series only addresses 32-bit userspace on 64-bit kernel support there are no associated userspace changes. Changing the 32-bit api here means that combining the KitKat branch from http://android.googlesource.com with a newer kernel version will not work. Is that something that anyone has said would work in the past? It seems that other parts of the Android userspace are pretty tied to specific kernel features / versions, is this anything new if the binder code had to change? We have explicitly said that Android userspace does not require a specific kernel version. I expect to see KitKat devices running at least 3.4, 3.10, and whatever is the next long term stable version. Sometimes new kernels need userspace support, but we try to avoid that as much as possible. The last major one I can remember was removing early suspend in 3.4, but in that case I provided a compat 3.4 branch to allow using 3.4 with an older userspace. Changing the binder api is not completely unsupportable for old versions, my guess is some people would just ship with a new kernel with the binder changes reverted. Anyway, the code as submitted has problems, see my response to the second patch, so it's not ready yet anyway :( thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/5] Staging: bcm: DDRInit: Fixed coding style issue, replaced spaces w/ tabs.(patch set)
This is the first patch of a series.
Replaced spaces in margin w/ 1 tab for lines:
11-15, 17-23, 25-58, 60, 62, 64
On branch staging-next
---
drivers/staging/bcm/DDRInit.c | 98 +--
1 file changed, 49 insertions(+), 49 deletions(-)
diff --git a/drivers/staging/bcm/DDRInit.c b/drivers/staging/bcm/DDRInit.c
index 9f7e30f..9ee844c 100644
--- a/drivers/staging/bcm/DDRInit.c
+++ b/drivers/staging/bcm/DDRInit.c
@@ -8,60 +8,60 @@
//DDR INIT-133Mhz
#define T3_SKIP_CLOCK_PROGRAM_DUMP_133MHZ 12 //index for 0x0F007000
static struct bcm_ddr_setting asT3_DDRSetting133MHz[]= {// # DPLL Clock
Setting
-{0x0F000800,0x7212},
-{0x0f000820,0x07F13FFF},
-{0x0f000810,0x0F95},
-{0x0f000860,0x},
-{0x0f000880,0x03DD},
+ {0x0F000800,0x7212},
+ {0x0f000820,0x07F13FFF},
+ {0x0f000810,0x0F95},
+ {0x0f000860,0x},
+ {0x0f000880,0x03DD},
// Changed source for X-bar and MIPS
clock to APLL
-{0x0f000840,0x0FFF1B00},
-{0x0f000870,0x0002},
-{0x0F00a044,0x1fff},
-{0x0F00a040,0x1f00},
-{0x0F00a084,0x1Cff},
-{0x0F00a080,0x1C00},
-{0x0F00a04C,0x000C},
+ {0x0f000840,0x0FFF1B00},
+ {0x0f000870,0x0002},
+ {0x0F00a044,0x1fff},
+ {0x0F00a040,0x1f00},
+ {0x0F00a084,0x1Cff},
+ {0x0F00a080,0x1C00},
+ {0x0F00a04C,0x000C},
//Memcontroller Default values
-{0x0F007000,0x00010001},
-{0x0F007004,0x01010100},
-{0x0F007008,0x0101},
-{0x0F00700c,0x},
-{0x0F007010,0x0100},
-{0x0F007014,0x01000100},
-{0x0F007018,0x0100},
-{0x0F00701c,0x01020001},// POP -
0x00020001 Normal 0x01020001
-{0x0F007020,0x04030107}, //Normal -
0x04030107 POP - 0x05030107
-{0x0F007024,0x0207},
-{0x0F007028,0x02020202},
-{0x0F00702c,0x0206060a},//ROB-
0x0205050a,//0x0206060a
-{0x0F007030,0x0500},
-{0x0F007034,0x0003},
-{0x0F007038,0x110a0200},//ROB -
0x110a0200,//0x180a0200,// 0x1f0a0200
-{0x0F00703C,0x02101010},//ROB -
0x02101010,//0x02101018},
-{0x0F007040,0x45751200},//ROB -
0x45751200,//0x450f1200},
-{0x0F007044,0x110a0d00},//ROB -
0x110a0d00//0x111f0d00
-{0x0F007048,0x081b0306},
-{0x0F00704c,0x},
-{0x0F007050,0x001c},
-{0x0F007054,0x},
-{0x0F007058,0x},
-{0x0F00705c,0x},
-{0x0F007060,0x0010246c},
-{0x0F007064,0x0010},
-{0x0F007068,0x},
-{0x0F00706c,0x0001},
-{0x0F007070,0x7000},
-{0x0F007074,0x},
-{0x0F007078,0x},
-{0x0F00707C,0x},
-{0x0F007080,0x},
-{0x0F007084,0x},
+ {0x0F007000,0x00010001},
+ {0x0F007004,0x01010100},
+ {0x0F007008,0x0101},
+ {0x0F00700c,0x},
+ {0x0F007010,0x0100},
+ {0x0F007014,0x01000100},
+ {0x0F007018,0x0100},
+ {0x0F00701c,0x01020001},// POP - 0x00020001 Normal 0x01020001
+ {0x0F007020,0x04030107}, //Normal - 0x04030107 POP - 0x05030107
+ {0x0F007024,0x0207},
+ {0x0F007028,0x02020202},
+ {0x0F00702c,0x0206060a},//ROB-
Re: [PATCH 1/3] vme_user: Ensure driver compiles after VME bridges
- Original Message -
From: Martyn Welch martyn.we...@ge.com
Sent: Thursday, November 7, 2013 7:00:35 AM
On 05/11/13 20:58, Aaron Sierra wrote:
- Original Message -
From: Martyn Welch martyn.we...@ge.com
Subject: Re: [PATCH 1/3] vme_user: Ensure driver compiles after VME
bridges
On 05/11/13 17:53, Aaron Sierra wrote:
Martyn,
Can you please elaborate on why you feel it is not a fix? For instance,
will this type of solution not be accepted upstream? Is this solution not
complete enough? Do you feel that it doesn't resolve any issue?
-Aaron
The vme_user driver is in the staging tree as it is not in a fit state to
incorporate into the main kernel tree. As far as I can see, the below
patch
just causes the driver to be built at a subtly different time, from a
subtly
different location (outside of the staging tree). To be honest, I'm not
sure
how this is fixing anything.
Martyn
Martyn,
You are correct about the function of this patch, however, I know that
it resolves a real problem despite its inconsequential appearance.
Using a 3.10.6 kernel, if I compile vme, vme_tsi148, and vme_user as
modules, then attempting to insert vme_user into the kernel before the
vme subsystem driver has been loaded results in the module failing to
load with the following (this is good):
vme_user: Unknown symbol vme_master_read (err 0)
vme_user: Unknown symbol vme_master_write (err 0)
vme_user: Unknown symbol vme_free_consistent (err 0)
vme_user: Unknown symbol vme_register_driver (err 0)
vme_user: Unknown symbol vme_irq_generate (err 0)
vme_user: Unknown symbol vme_alloc_consistent (err 0)
vme_user: Unknown symbol vme_slave_request (err 0)
vme_user: Unknown symbol vme_master_free (err 0)
vme_user: Unknown symbol vme_master_request (err 0)
vme_user: Unknown symbol vme_slave_free (err 0)
vme_user: Unknown symbol vme_slave_set (err 0)
vme_user: Unknown symbol vme_master_get (err 0)
vme_user: Unknown symbol vme_slave_get (err 0)
vme_user: Unknown symbol vme_master_set (err 0)
vme_user: Unknown symbol vme_unregister_driver (err 0)
vme_user: Unknown symbol vme_get_size (err 0)
This Unknown symbol error forces modules to be loaded in this order
at the very least; vme - vme_user.
However, when these modules are compiled into the kernel, unfortunate
things start to take place. This is the order that the three VME
drivers are compiled when using an unpatched kernel:
CC drivers/staging/vme/devices/vme_user.o
LD drivers/staging/vme/devices/built-in.o
LD drivers/staging/vme/built-in.o
LD drivers/staging/built-in.o
CC drivers/vme/vme.o
CC drivers/vme/bridges/vme_tsi148.o
LD drivers/vme/bridges/built-in.o
LD drivers/vme/built-in.o
LD drivers/built-in.o
Note that this is not only the order that the drivers are compiled, but
also the order that they are initialized. Also notice that since all
three drivers are compiled into the kernel, there are no Unknown
symbols to prevent the vme_user driver from initializing as if the VME
bus subsystem has already been initialized. Therefore the following load
order occurs; vme_user - vme - vme_tsi148.
This is the result of the unpatched initialization order:
[snip]
hidraw: raw HID events driver (C) Jiri Kosina
usbcore: registered new interface driver usbhid
usbhid: USB HID core driver
vme_user: VME User Space Access Driver
[ cut here ]
kernel BUG at drivers/base/driver.c:169!
invalid opcode: [#1] SMP
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted
3.10.6-xes_r3-00018-g300fcda-dirty #112
Hardware name: Extreme Engineering Solutions, Inc. (X-ES)
XCalibur4331/XCalibur4331, BIOS 1-2.21.1 12/10/2012
task: 880232888000 ti: 880232884000 task.ti: 880232884000
RIP: 0010:[812ba23a] [812ba23a]
driver_register+0x1d/0x106
RSP: :880232885e18 EFLAGS: 00010246
RAX: 8184ef20 RBX: 8184ee68 RCX: 0026
RDX: 0023 RSI: 0020 RDI: 8184ee68
RBP: 880232885e48 R08: 0001 R09:
R10: R11: 81d5f420 R12: 8184ee30
R13: R14: R15: 8184eee0
FS: () GS:88023bc8()
knlGS:
CS: 0010 DS: ES: CR0: 8005003b
CR2: CR3: 0180c000 CR4: 07e0
DR0: DR1: DR2:
DR3: DR6: 0ff0 DR7: 0400
Stack:
0001 818a576d 8184ee30
8184eee0 880232885eb8 813a63aa
880232885eb8 814bc12b 880232885eb8 0028
Call Trace:
[PATCH 2/5] Staging: bcm:DDRInit: Fixed coding style issue, replaced spaces w/ tab.(patch set)
This is the second patch of a series.(2)
Replaced spaces in margin w/ 1 tab for lines:
69-78, 80-114, 116
Signed-off-by: Gary Alan Rookard garyrook...@gmail.com
On branch staging-next
---
drivers/staging/bcm/DDRInit.c | 92 +--
1 file changed, 46 insertions(+), 46 deletions(-)
diff --git a/drivers/staging/bcm/DDRInit.c b/drivers/staging/bcm/DDRInit.c
index 9ee844c..6f5880b 100644
--- a/drivers/staging/bcm/DDRInit.c
+++ b/drivers/staging/bcm/DDRInit.c
@@ -66,54 +66,54 @@ static struct bcm_ddr_setting asT3_DDRSetting133MHz[]= {//
# DPLL Clock Set
//80Mhz
#define T3_SKIP_CLOCK_PROGRAM_DUMP_80MHZ 10 //index for 0x0F007000
static struct bcm_ddr_setting asT3_DDRSetting80MHz[]= {// # DPLL Clock
Setting
-{0x0f000810,0x0F95},
-{0x0f000820,0x07f1},
-{0x0f000860,0x},
-{0x0f000880,0x03DD},
-{0x0F00a044,0x1fff},
-{0x0F00a040,0x1f00},
-{0x0F00a084,0x1Cff},
-{0x0F00a080,0x1C00},
-{0x0F00a000,0x0016},
-{0x0F00a04C,0x000C},
+ {0x0f000810,0x0F95},
+ {0x0f000820,0x07f1},
+ {0x0f000860,0x},
+ {0x0f000880,0x03DD},
+ {0x0F00a044,0x1fff},
+ {0x0F00a040,0x1f00},
+ {0x0F00a084,0x1Cff},
+ {0x0F00a080,0x1C00},
+ {0x0F00a000,0x0016},
+ {0x0F00a04C,0x000C},
//Memcontroller Default values
-{0x0F007000,0x00010001},
-{0x0F007004,0x0100},
-{0x0F007008,0x0101},
-{0x0F00700c,0x},
-{0x0F007010,0x0100},
-{0x0F007014,0x01000100},
-{0x0F007018,0x0100},
-{0x0F00701c,0x0102},
-{0x0F007020,0x04020107},
-{0x0F007024,0x0007},
-{0x0F007028,0x02020201},
-{0x0F00702c,0x0204040a},
-{0x0F007030,0x0400},
-{0x0F007034,0x0002},
-{0x0F007038,0x1F060200},
-{0x0F00703C,0x1C1F},
-{0x0F007040,0x8A006600},
-{0x0F007044,0x221a0800},
-{0x0F007048,0x02690204},
-{0x0F00704c,0x},
-{0x0F007050,0x001c},
-{0x0F007054,0x},
-{0x0F007058,0x},
-{0x0F00705c,0x},
-{0x0F007060,0x000A15D6},
-{0x0F007064,0x000A},
-{0x0F007068,0x},
-{0x0F00706c,0x0001},
-{0x0F007070,0x4000},
-{0x0F007074,0x},
-{0x0F007078,0x},
-{0x0F00707C,0x},
-{0x0F007080,0x},
-{0x0F007084,0x},
-{0x0F007094,0x0104},
+ {0x0F007000,0x00010001},
+ {0x0F007004,0x0100},
+ {0x0F007008,0x0101},
+ {0x0F00700c,0x},
+ {0x0F007010,0x0100},
+ {0x0F007014,0x01000100},
+ {0x0F007018,0x0100},
+ {0x0F00701c,0x0102},
+{0x0F007020,0x04020107},
+{0x0F007024,0x0007},
+ {0x0F007028,0x02020201},
+ {0x0F00702c,0x0204040a},
+ {0x0F007030,0x0400},
+ {0x0F007034,0x0002},
+ {0x0F007038,0x1F060200},
+ {0x0F00703C,0x1C1F},
+ {0x0F007040,0x8A006600},
+ {0x0F007044,0x221a0800},
+ {0x0F007048,0x02690204},
+ {0x0F00704c,0x},
+ {0x0F007050,0x001c},
+ {0x0F007054,0x},
+ {0x0F007058,0x},
+ {0x0F00705c,0x},
+ {0x0F007060,0x000A15D6},
+ {0x0F007064,0x000A},
+
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
None of this (the patch series or the original code) is mine. My Sorry mistraced the attribution sequence question was more of a general one on designing ioctls, as well as concerns with changing the existing 32-bit api. I think in general my advice would be: If its already been screwed up - sort the structures out for the 64bit version - make the 32bit version and compat ones clean copies of what they are now - don't try and pull stunts with alignof and other such tricks because - some poor bugger will have to debug it one day (and it might be you some years after you forget how it worked) - most of our security holes show up in complex data parsing paths - figure out whether its better to do compat fixups or just have 64bit use new structures and new ioctl numbers with the 32bit ones working but with 32bit offsets regardless of 32/64bit CPU mode. and for new stuff - design for 64bit safety in advance - watch the padding rules and user alignment - consider leaving some spare zero space on the end of the structs Alan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/5] Staging: bcm: DDRInit: Fixed coding style issue, replaced spaces w/ tab.(patch set)
This is the third patch of a series.(3)
Replaced spaces in margin w/ 1 tab for lines:
121-125, 128-134, 136, 138-171, 173, 175
Signed-off-by: Gary Alan Rookard garyrook...@gmail.com
On branch staging-next
---
drivers/staging/bcm/DDRInit.c | 98 +--
1 file changed, 49 insertions(+), 49 deletions(-)
diff --git a/drivers/staging/bcm/DDRInit.c b/drivers/staging/bcm/DDRInit.c
index 6f5880b..d31dc71 100644
--- a/drivers/staging/bcm/DDRInit.c
+++ b/drivers/staging/bcm/DDRInit.c
@@ -118,61 +118,61 @@ static struct bcm_ddr_setting asT3_DDRSetting80MHz[]= {//
# DPLL Clock Setting
//100Mhz
#define T3_SKIP_CLOCK_PROGRAM_DUMP_100MHZ 13 //index for 0x0F007000
static struct bcm_ddr_setting asT3_DDRSetting100MHz[]= {// # DPLL Clock
Setting
-{0x0F000800,0x7008},
-{0x0f000810,0x0F95},
-{0x0f000820,0x07F13E3F},
-{0x0f000860,0x},
-{0x0f000880,0x03DD},
+ {0x0F000800,0x7008},
+ {0x0f000810,0x0F95},
+ {0x0f000820,0x07F13E3F},
+ {0x0f000860,0x},
+ {0x0f000880,0x03DD},
// Changed source for X-bar and MIPS clock to
APLL
//0x0f000840,0x0FFF1800,
-{0x0f000840,0x0FFF1B00},
-{0x0f000870,0x0002},
-{0x0F00a044,0x1fff},
-{0x0F00a040,0x1f00},
-{0x0F00a084,0x1Cff},
-{0x0F00a080,0x1C00},
-{0x0F00a04C,0x000C},
+ {0x0f000840,0x0FFF1B00},
+ {0x0f000870,0x0002},
+ {0x0F00a044,0x1fff},
+ {0x0F00a040,0x1f00},
+ {0x0F00a084,0x1Cff},
+ {0x0F00a080,0x1C00},
+ {0x0F00a04C,0x000C},
//# Enable 2 ports within X-bar
-{0x0F00A000,0x0016},
+ {0x0F00A000,0x0016},
//Memcontroller Default values
-{0x0F007000,0x00010001},
-{0x0F007004,0x01010100},
-{0x0F007008,0x0101},
-{0x0F00700c,0x},
-{0x0F007010,0x0100},
-{0x0F007014,0x01000100},
-{0x0F007018,0x0100},
-{0x0F00701c,0x01020001}, // POP -
0x0002 Normal 0x0102
-{0x0F007020,0x04020107},//Normal -
0x04030107 POP - 0x05030107
-{0x0F007024,0x0007},
-{0x0F007028,0x01020201},
-{0x0F00702c,0x0204040A},
-{0x0F007030,0x0600},
-{0x0F007034,0x0004},
-{0x0F007038,0x20080200},
-{0x0F00703C,0x02030320},
-{0x0F007040,0x6E7F1200},
-{0x0F007044,0x01190A00},
-{0x0F007048,0x06120305},//0x02690204
// 0x06120305
-{0x0F00704c,0x},
-{0x0F007050,0x001C},
-{0x0F007054,0x},
-{0x0F007058,0x},
-{0x0F00705c,0x},
-{0x0F007060,0x00082ED6},
-{0x0F007064,0x000A},
-{0x0F007068,0x},
-{0x0F00706c,0x0001},
-{0x0F007070,0x5000},
-{0x0F007074,0x},
-{0x0F007078,0x},
-{0x0F00707C,0x},
-{0x0F007080,0x},
-{0x0F007084,0x},
+ {0x0F007000,0x00010001},
+ {0x0F007004,0x01010100},
+ {0x0F007008,0x0101},
+ {0x0F00700c,0x},
+ {0x0F007010,0x0100},
+ {0x0F007014,0x01000100},
+ {0x0F007018,0x0100},
+ {0x0F00701c,0x01020001}, // POP - 0x0002 Normal 0x0102
[PATCH 3/3] Staging: bcm: DDRInit: Fixed coding style issue, replaced spaces w/ tab.(patch set)
This is the third patch of a series.(3)
Replaced spaces in margin w/ 1 tab for lines:
121-125, 128-134, 136, 138-171, 173, 175
Signed-off-by: Gary Alan Rookard garyrook...@gmail.com
On branch staging-next
---
drivers/staging/bcm/DDRInit.c | 98 +--
1 file changed, 49 insertions(+), 49 deletions(-)
diff --git a/drivers/staging/bcm/DDRInit.c b/drivers/staging/bcm/DDRInit.c
index 6f5880b..d31dc71 100644
--- a/drivers/staging/bcm/DDRInit.c
+++ b/drivers/staging/bcm/DDRInit.c
@@ -118,61 +118,61 @@ static struct bcm_ddr_setting asT3_DDRSetting80MHz[]= {//
# DPLL Clock Setting
//100Mhz
#define T3_SKIP_CLOCK_PROGRAM_DUMP_100MHZ 13 //index for 0x0F007000
static struct bcm_ddr_setting asT3_DDRSetting100MHz[]= {// # DPLL Clock
Setting
-{0x0F000800,0x7008},
-{0x0f000810,0x0F95},
-{0x0f000820,0x07F13E3F},
-{0x0f000860,0x},
-{0x0f000880,0x03DD},
+ {0x0F000800,0x7008},
+ {0x0f000810,0x0F95},
+ {0x0f000820,0x07F13E3F},
+ {0x0f000860,0x},
+ {0x0f000880,0x03DD},
// Changed source for X-bar and MIPS clock to
APLL
//0x0f000840,0x0FFF1800,
-{0x0f000840,0x0FFF1B00},
-{0x0f000870,0x0002},
-{0x0F00a044,0x1fff},
-{0x0F00a040,0x1f00},
-{0x0F00a084,0x1Cff},
-{0x0F00a080,0x1C00},
-{0x0F00a04C,0x000C},
+ {0x0f000840,0x0FFF1B00},
+ {0x0f000870,0x0002},
+ {0x0F00a044,0x1fff},
+ {0x0F00a040,0x1f00},
+ {0x0F00a084,0x1Cff},
+ {0x0F00a080,0x1C00},
+ {0x0F00a04C,0x000C},
//# Enable 2 ports within X-bar
-{0x0F00A000,0x0016},
+ {0x0F00A000,0x0016},
//Memcontroller Default values
-{0x0F007000,0x00010001},
-{0x0F007004,0x01010100},
-{0x0F007008,0x0101},
-{0x0F00700c,0x},
-{0x0F007010,0x0100},
-{0x0F007014,0x01000100},
-{0x0F007018,0x0100},
-{0x0F00701c,0x01020001}, // POP -
0x0002 Normal 0x0102
-{0x0F007020,0x04020107},//Normal -
0x04030107 POP - 0x05030107
-{0x0F007024,0x0007},
-{0x0F007028,0x01020201},
-{0x0F00702c,0x0204040A},
-{0x0F007030,0x0600},
-{0x0F007034,0x0004},
-{0x0F007038,0x20080200},
-{0x0F00703C,0x02030320},
-{0x0F007040,0x6E7F1200},
-{0x0F007044,0x01190A00},
-{0x0F007048,0x06120305},//0x02690204
// 0x06120305
-{0x0F00704c,0x},
-{0x0F007050,0x001C},
-{0x0F007054,0x},
-{0x0F007058,0x},
-{0x0F00705c,0x},
-{0x0F007060,0x00082ED6},
-{0x0F007064,0x000A},
-{0x0F007068,0x},
-{0x0F00706c,0x0001},
-{0x0F007070,0x5000},
-{0x0F007074,0x},
-{0x0F007078,0x},
-{0x0F00707C,0x},
-{0x0F007080,0x},
-{0x0F007084,0x},
+ {0x0F007000,0x00010001},
+ {0x0F007004,0x01010100},
+ {0x0F007008,0x0101},
+ {0x0F00700c,0x},
+ {0x0F007010,0x0100},
+ {0x0F007014,0x01000100},
+ {0x0F007018,0x0100},
+ {0x0F00701c,0x01020001}, // POP - 0x0002 Normal 0x0102
[PATCH 5/5] Staging: bcm: DDRInit: Fixed coding style issue, replaced spaces w/ tab.(patch set)
This is the fifth and final patch for replace spaces w/tab.(5)
Replaced spaces in margin w/ 1 tab for lines:
193-197, 199-200, 202-205, 207, 209-242, 244, 246
Signed-off-by: Gary Alan Rookard garyrook...@gmail.com
On branch staging-next
---
drivers/staging/bcm/DDRInit.c | 96 +--
1 file changed, 48 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/bcm/DDRInit.c b/drivers/staging/bcm/DDRInit.c
index fdeb4f6..dc14cf5 100644
--- a/drivers/staging/bcm/DDRInit.c
+++ b/drivers/staging/bcm/DDRInit.c
@@ -190,60 +190,60 @@ static struct bcm_ddr_setting asDPLL_266MHZ[] = {
#define T3B_SKIP_CLOCK_PROGRAM_DUMP_133MHZ 11 //index for 0x0F007000
static struct bcm_ddr_setting asT3B_DDRSetting133MHz[] = {// # DPLL Clock
Setting
-{0x0f000810,0x0F95},
-{0x0f000810,0x0F95},
-{0x0f000810,0x0F95},
-{0x0f000820,0x07F13652},
-{0x0f000840,0x0FFF0800},
+ {0x0f000810,0x0F95},
+ {0x0f000810,0x0F95},
+ {0x0f000810,0x0F95},
+ {0x0f000820,0x07F13652},
+ {0x0f000840,0x0FFF0800},
// Changed source for X-bar and MIPS
clock to APLL
-{0x0f000880,0x03DD},
-{0x0f000860,0x},
+ {0x0f000880,0x03DD},
+ {0x0f000860,0x},
// Changed source for X-bar and MIPS
clock to APLL
-{0x0F00a044,0x1fff},
-{0x0F00a040,0x1f00},
-{0x0F00a084,0x1Cff},
-{0x0F00a080,0x1C00},
+ {0x0F00a044,0x1fff},
+ {0x0F00a040,0x1f00},
+ {0x0F00a084,0x1Cff},
+ {0x0F00a080,0x1C00},
//# Enable 2 ports within X-bar
-{0x0F00A000,0x0016},
+ {0x0F00A000,0x0016},
//Memcontroller Default values
-{0x0F007000,0x00010001},
-{0x0F007004,0x01010100},
-{0x0F007008,0x0101},
-{0x0F00700c,0x},
-{0x0F007010,0x0100},
-{0x0F007014,0x01000100},
-{0x0F007018,0x0100},
-{0x0F00701c,0x01020001},// POP -
0x00020001 Normal 0x01020001
-{0x0F007020,0x04030107}, //Normal -
0x04030107 POP - 0x05030107
-{0x0F007024,0x0207},
-{0x0F007028,0x02020202},
-{0x0F00702c,0x0206060a},//ROB-
0x0205050a,//0x0206060a
-{0x0F007030,0x0500},
-{0x0F007034,0x0003},
-{0x0F007038,0x130a0200},//ROB -
0x110a0200,//0x180a0200,// 0x1f0a0200
-{0x0F00703C,0x02101012},//ROB -
0x02101010,//0x02101018},
-{0x0F007040,0x457D1200},//ROB -
0x45751200,//0x450f1200},
-{0x0F007044,0x11130d00},//ROB -
0x110a0d00//0x111f0d00
-{0x0F007048,0x040D0306},
-{0x0F00704c,0x},
-{0x0F007050,0x001c},
-{0x0F007054,0x},
-{0x0F007058,0x},
-{0x0F00705c,0x},
-{0x0F007060,0x0010246c},
-{0x0F007064,0x0012},
-{0x0F007068,0x},
-{0x0F00706c,0x0001},
-{0x0F007070,0x7000},
-{0x0F007074,0x},
-{0x0F007078,0x},
-{0x0F00707C,0x},
-{0x0F007080,0x},
-{0x0F007084,0x},
+ {0x0F007000,0x00010001},
+ {0x0F007004,0x01010100},
+ {0x0F007008,0x0101},
+ {0x0F00700c,0x},
+ {0x0F007010,0x0100},
+
Re: [PATCH 04/11] staging: et131x: drop packet when error occurs in et131x_tx
On Thu, Dec 5, 2013 at 7:23 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 03:24:14PM +0800, ZHAO Gang wrote: As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY when tx failed. Really? That's ok to do? Seems like you are changing the logic of the function a lot here, how does the code let userspace know packets were dropped then? The TODO file said: - In et131x_tx(), don't return NETDEV_TX_BUSY, just drop the packet with kfree_skb(). I think I did what the TODO file suggested, the real logic change is when tx_ring is full, instead of return NETDEV_TX_BUSY, drop packet and return NETDEV_TX_OK. But a `git grep` shows that many ethernet drivers return NETDEV_TX_BUSY, maybe the best practice is to return NETDEV_TX_BUSY ? If it is, we should remove this item from TODO file. greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/5] Staging: bcm: DDRInit: Fixed coding style issue, repaced spaces w/ tab.(patch set)
This is the fourth patch of a series.(4)
Replaced spaces in margin w/ 1 tab for lines:
181-185, 187-188
Signed-off-by: Gary Alan Rookard garyrook...@gmail.com
On branch staging-next
---
drivers/staging/bcm/DDRInit.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/bcm/DDRInit.c b/drivers/staging/bcm/DDRInit.c
index d31dc71..fdeb4f6 100644
--- a/drivers/staging/bcm/DDRInit.c
+++ b/drivers/staging/bcm/DDRInit.c
@@ -178,14 +178,14 @@ static struct bcm_ddr_setting asT3_DDRSetting100MHz[]=
{// # DPLL Clock Setting
//Net T3B DDR Settings
//DDR INIT-133Mhz
static struct bcm_ddr_setting asDPLL_266MHZ[] = {
-{0x0F000800,0x7212},
-{0x0f000820,0x07F13FFF},
-{0x0f000810,0x0F95},
-{0x0f000860,0x},
-{0x0f000880,0x03DD},
+ {0x0F000800,0x7212},
+ {0x0f000820,0x07F13FFF},
+ {0x0f000810,0x0F95},
+ {0x0f000860,0x},
+ {0x0f000880,0x03DD},
// Changed source for X-bar and MIPS
clock to APLL
-{0x0f000840,0x0FFF1B00},
-{0x0f000870,0x0002}
+ {0x0f000840,0x0FFF1B00},
+ {0x0f000870,0x0002}
};
#define T3B_SKIP_CLOCK_PROGRAM_DUMP_133MHZ 11 //index for 0x0F007000
--
1.8.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/11] staging: et131x: simplify rx dma code
On Wed, Dec 4, 2013 at 10:53 PM, Mark Einon mark.ei...@gmail.com wrote: On Wed, Dec 04, 2013 at 09:32:27PM +0800, ZHAO Gang wrote: By re-examine the code I found the calculation is not correct. The real impact is too big to apply this patch, so this patch and following patches should be dropped. This will mess up the patch numbering for your set (e.g. only 7 of 11 are to be applied), which will annoy Greg, in my experience. Make sure the whole set is dropped by explicitly asking Greg to do so, and re-send the patches that you do want to submit, numbered correctly. Some patches are questionable, some have been applied. I think I'd better do some changes and resend questionable patches. Cheers, Mark ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 9/9] staging: android: binder: Add binder compat layer
On Wed, Dec 4, 2013 at 2:02 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 01:55:34PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 1:43 PM, Greg KH gre...@linuxfoundation.org wrote: On Wed, Dec 04, 2013 at 12:46:42PM -0800, Colin Cross wrote: On Wed, Dec 4, 2013 at 10:35 AM, Greg KH gre...@linuxfoundation.org wrote: snip And finally, is this all really needed? Why not just fix the structures to be correct, and then fix userspace to use the correct structures as well, thereby not needing a compat layer at all? Some of the binder ioctls take userspace pointers. Are you suggesting storing those pointers in a __u64 to avoid having to have a compat_ioctl? Yes, that's the best way to solve the issue, right? It's the least code, but in exchange you lose all the type safety and warnings when copying in and out of the pointers, as well as sparse checking on the __user attribute. Not if you make the cast right at the beginning, when you first touch the data, but yes, it does take some of the type saftey away, at the expense of simpler code to mess up :) That doesn't seem like a good tradeoff to me. In addition it requires modifying the existing heavily used 32 bit api, which means a mostly-equivalent compat layer added in libbinder to support old kernels. Wait, I thought that libbinder would have to be changed anyway here, to handle 64bit kernels (in both 32 and 64bit userspace). Since you are already changing it, why not just do it correctly? Yes libbinder will have to be changed to support calls between 32 bit and 64 bit processes, so I don't see much value in a patchset that only supports all 32 bit or all 64 bit processes. If user space is fixed to use 64 bit pointers on a 64 bit system, then much of the code added in this patchset becomes useless (and probably harmful as it appears to prevent 32 bit processes from communicating with 64 bit processes). Or does this patch series mean that no userspace code is changed? Is that a requirement here? I don't think we need to support old 32 bit userspace framework code on a 64 bit system. I think it is more important to not prevent mixed mode systems. -- Arve Hjønnevåg ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] checkpatch: Warn only on space before semicolon at end of line
Thanks so much. :) regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
