* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Jonathan Cameron @ 2013-09-08 13:42 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1378034277-26728-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 09/01/13 12:17, Zubair Lutfullah wrote:
> Previously the driver had only one-shot reading functionality.
> This patch adds triggered buffer support to the driver.
>
> Continuous sampling starts when buffer is enabled.
> And samples are pushed to userpace by the trigger which
> triggers automatically at every hardware interrupt
> of FIFO1 filling with samples upto threshold value.
>
> Userspace responsibility to stop sampling by writing zero
> in the buffer enable file.
>
> Patil Rachna (TI) laid the ground work for ADC HW register access.
> Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
>
> I fixed channel scanning so multiple ADC channels can be read
> simultaneously and pushed to userspace.
> Restructured the driver to fit IIO ABI.
> And added trigger support.
>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Russ Dill <Russ.Dill-l0cyMroinI0@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Hi
I'm afraid I'm still unconvinced that we can use the trigger infrastructure
as done here. This trigger from a fifo threshold does not agree with how it
happens in any other driver. Unfortunately as the first hardware buffered
device we have had in recent times (there is the sca3000 in staging, but that
does not have a front end kfifo) you get to break some new ground.
I've suggested below how I would handle this. In short, drop the trigger
and drive the buffers directly, marking the mode as INDIO_BUFFER_HARDWARE to
avoid the core code complaining at the lack of trigger. It will end up simpler
and not expose a trigger to userspace that is doing 'interesting' things.
This is the only way I can envision a hardware buffer equiped device like this
working. It's certainly what I've been intending to do with the sca3000
driver when I finally get around to bringing it up to date. I don't think
we can take the current approach because it would give us horribly inconsistent
userspace interfaces we can't maintain in the long run.
Few other little bits and bobs inline to do with unused variables and
a few places where a small comment or two might make the operation easier
to follow when we have all forgotten how the hardware works ;)
Jonathan
> ---
> drivers/iio/adc/Kconfig | 1 +
> drivers/iio/adc/ti_am335x_adc.c | 243 +++++++++++++++++++++++++++++++---
> include/linux/mfd/ti_am335x_tscadc.h | 13 ++
> 3 files changed, 237 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 09371cb..cfa2039 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -167,6 +167,7 @@ config TI_ADC081C
> config TI_AM335X_ADC
> tristate "TI's AM335X ADC driver"
> depends on MFD_TI_AM335X_TSCADC
> + select IIO_TRIGGERED_BUFFER
> help
> Say yes here to build support for Texas Instruments ADC
> driver which is also a MFD client.
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index a952538..1626e16 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -24,16 +24,23 @@
> #include <linux/iio/iio.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
Ouch, how did that slip in here. Dropping this should have been
in a separate patch as it doesn't really have anything to do with
the rest of this series.
> -#include <linux/iio/machine.h>
> #include <linux/iio/driver.h>
>
> #include <linux/mfd/ti_am335x_tscadc.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> struct tiadc_device {
> struct ti_tscadc_dev *mfd_tscadc;
> int channels;
> u8 channel_line[8];
> u8 channel_step[8];
> + int irq;
> + int buffer_en_ch_steps;
> + struct iio_trigger *trig;
> + u32 *data;
> };
>
> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,10 +63,16 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
> return step_en;
> }
>
> -static void tiadc_step_config(struct tiadc_device *adc_dev)
> +static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
> {
> + return 1 << adc_dev->channel_step[chan];
> +}
> +
> +static void tiadc_step_config(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> unsigned int stepconfig;
> - int i, steps;
> + int i, steps, chan;
Whilst I agree that pulling chan up here is slightly cleaner, technically that
is a bit of unconnected cleanup and should have been in a precursor patch.
(though don't bother doing so now!)
>
> /*
> * There are 16 configurable steps and 8 analog input
> @@ -72,11 +85,13 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> */
>
> steps = TOTAL_STEPS - adc_dev->channels;
> - stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> + if (iio_buffer_enabled(indio_dev))
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
> + | STEPCONFIG_MODE_SWCNT;
> + else
> + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>
> for (i = 0; i < adc_dev->channels; i++) {
> - int chan;
> -
> chan = adc_dev->channel_line[i];
> tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
> stepconfig | STEPCONFIG_INP(chan));
> @@ -85,7 +100,167 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> adc_dev->channel_step[i] = steps;
> steps++;
> }
> +}
> +
> +static irqreturn_t tiadc_irq(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + unsigned int status, config;
> + status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> + /*
> + * ADC and touchscreen share the IRQ line.
> + * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
> + */
> + if (status & IRQENB_FIFO1OVRRUN) {
> + /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> + config = tiadc_readl(adc_dev, REG_CTRL);
> + config &= ~(CNTRLREG_TSCSSENB);
> + tiadc_writel(adc_dev, REG_CTRL, config);
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
> + | IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
> + return IRQ_HANDLED;
> + } else if (status & IRQENB_FIFO1THRES) {
> + /* Trigger to push FIFO data to iio buffer */
> + tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
So this is triggering at the threshold. Thus it isn't acting as a conventional
trigger in the IIO sense at all. It is of no real use to anything outside
this driver, so at the very least add the two callbacks to prevent this
driver using any other trigger and the trigger being used by any other device.
However, I'm really not happy with this solution.
Take a look at running with no trigger at all and calling the buffer
filling more directly. The issue is that if we have a trigger now it will
become part of the userspace interface and become a pain to drop later.
It is valid here I think to use the INDIO_BUFFER_HARDWARE mode (which skips
the checks for a trigger) then just don't call iio_triggered_buffer_post_enable
or iio_triggered_buffer_predisable. Then make this a threaded interrupt and
put your tiadc_trigger_h as the thread. Finally just return IRQ_WAKE_THREAD
in this case. You'll have to pull a few bits out of
industrialio-triggered-buffer.c to create the kfifo etc, but no more than
a few lines given the pollfunc won't exist.
Actually, might be cleaner to move the flush case into the thread as well
and just have a single check in this top half of the handler. That bit
is up to you given it is really a question of how long it take and hence
if it is acceptable in interrupt context.
> + iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
> + return IRQ_HANDLED;
> + } else
> + return IRQ_NONE;
> +
> +}
> +
> +static irqreturn_t tiadc_trigger_h(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int i, k, fifo1count, read;
> + u32 *data = adc_dev->data;
> +
Hmm.. This results in all data in the fifo being read from a single
trigger. I think I would still be happier if in the case of hardware
buffers like this we didn't use a visible trigger at all. Such
a trigger has a somewhat fuzzy meaning to say the least.
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (k = 0; k < fifo1count; k = k + i) {
As mentioned below, why not use 16 bit storage in the kfifo?
> + for (i = 0; i < (indio_dev->scan_bytes)/4; i++) {
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> + data[i] = read & FIFOREAD_DATA_MASK;
> + }
> + iio_push_to_buffers(indio_dev, (u8 *) data);
> + }
>
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int i, fifo1count, read;
> +
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN |
> + IRQENB_FIFO1UNDRFLW));
> +
Why would there be data in the fifo? Seems to me that flushing
after disabling it and before enabling it should not both be
necessary? Is this just a case of playing safe?
> + /* Flush FIFO before starting sampling */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return iio_sw_buffer_preenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + struct iio_buffer *buffer = indio_dev->buffer;
> + unsigned int enb = 0;
> + u8 bit;
> +
> + adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> + if (adc_dev->data == NULL)
> + return -ENOMEM;
> +
> + tiadc_step_config(indio_dev);
> + for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
> + enb |= (get_adc_step_bit(adc_dev, bit) << 1);
> + adc_dev->buffer_en_ch_steps = enb;
> +
> + am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
> +
> + tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES
> + | IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> + tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES
> + | IRQENB_FIFO1OVRRUN);
> +
> + return iio_triggered_buffer_postenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int fifo1count, i, read;
> +
> + tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> + IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
> + am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
> +
> + /* Flush FIFO of any leftover data */
> + fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> + for (i = 0; i < fifo1count; i++)
> + read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> + return iio_triggered_buffer_predisable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +
> + tiadc_step_config(indio_dev);
> + kfree(adc_dev->data);
blank line here please.
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
> + .preenable = &tiadc_buffer_preenable,
> + .postenable = &tiadc_buffer_postenable,
> + .predisable = &tiadc_buffer_predisable,
> + .postdisable = &tiadc_buffer_postdisable,
> +};
> +
> +static const struct iio_trigger_ops tiadc_trigger_ops = {
> + .owner = THIS_MODULE,
> +};
> +
If you want a utility function for allocation, convention would
say have one for freeing as well so that the code is obviously
balanced and easy to check. Also this registers the buffer as
well so the name should probably reflect that.
> +static int tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> +{
> + struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + struct iio_trigger *trig = adc_dev->trig;
> + int ret;
> +
devm_iio_trigger_alloc is now available and will simplify things a little.
> + trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> + if (trig == NULL)
> + return -ENOMEM;
> +
> + trig->dev.parent = indio_dev->dev.parent;
> + trig->ops = &tiadc_trigger_ops;
> + iio_trigger_set_drvdata(trig, indio_dev);
> +
> + ret = iio_trigger_register(trig);
> + if (ret)
> + goto err_free_trigger;
> +
> + return 0;
> +
> +err_free_trigger:
> + iio_trigger_free(adc_dev->trig);
> +
> + return ret;
> }
>
> static const char * const chan_name_ain[] = {
> @@ -120,6 +295,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> chan->channel = adc_dev->channel_line[i];
> chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> chan->datasheet_name = chan_name_ain[chan->channel];
> + chan->scan_index = i;
> chan->scan_type.sign = 'u';
> chan->scan_type.realbits = 12;
> chan->scan_type.storagebits = 32;
I never noticed this before but putting 12 bit values in 32 bit storage
does seem rather wasteful. Far as I can see above there is no real
reason for doing so. If there is, then please add a comment justifying this
choice!
> @@ -142,11 +318,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> int i, map_val;
> unsigned int fifo1count, read, stepid;
> - u32 step = UINT_MAX;
> bool found = false;
> u32 step_en;
> unsigned long timeout = jiffies + usecs_to_jiffies
> (IDLE_TIMEOUT * adc_dev->channels);
> +
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> +
> step_en = get_adc_step_mask(adc_dev);
> am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
>
> @@ -168,15 +347,6 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> * Hence we need to flush out this data.
> */
Is the comment above still valid? I'd guess it referred to this code
which has now gone.
>
> - for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
> - if (chan->channel == adc_dev->channel_line[i]) {
> - step = adc_dev->channel_step[i];
> - break;
> - }
> - }
> - if (WARN_ON_ONCE(step == UINT_MAX))
> - return -EINVAL;
> -
> fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> for (i = 0; i < fifo1count; i++) {
> read = tiadc_readl(adc_dev, REG_FIFO1);
> @@ -231,26 +401,49 @@ static int tiadc_probe(struct platform_device *pdev)
> channels++;
> }
> adc_dev->channels = channels;
Use the devm interfaces and there is no need to keep a copy
of the irq number about.
> + adc_dev->irq = adc_dev->mfd_tscadc->irq;
>
> indio_dev->dev.parent = &pdev->dev;
> indio_dev->name = dev_name(&pdev->dev);
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->info = &tiadc_info;
>
> - tiadc_step_config(adc_dev);
> + tiadc_step_config(indio_dev);
> + tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
>
> err = tiadc_channel_init(indio_dev, adc_dev->channels);
> if (err < 0)
> return err;
>
> - err = iio_device_register(indio_dev);
> + err = tiadc_iio_allocate_trigger(indio_dev);
> if (err)
> goto err_free_channels;
>
devm_request_irq will simplify error handling a little.
> + err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
> + indio_dev->name, indio_dev);
> + if (err)
> + goto err_unregister_trigger;
> +
> + err = iio_triggered_buffer_setup(indio_dev, NULL,
> + &tiadc_trigger_h, &tiadc_buffer_setup_ops);
> + if (err)
> + goto err_free_irq;
> +
> + err = iio_device_register(indio_dev);
> + if (err)
> + goto err_buffer_unregister;
> +
> platform_set_drvdata(pdev, indio_dev);
>
> return 0;
>
> +err_buffer_unregister:
> + iio_buffer_unregister(indio_dev);
> +err_free_irq:
> + free_irq(adc_dev->irq, indio_dev);
> +err_unregister_trigger:
> + iio_trigger_unregister(adc_dev->trig);
> + iio_trigger_free(adc_dev->trig);
> err_free_channels:
> tiadc_channels_remove(indio_dev);
> return err;
> @@ -262,7 +455,11 @@ static int tiadc_remove(struct platform_device *pdev)
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> u32 step_en;
>
> + iio_trigger_unregister(adc_dev->trig);
> + iio_trigger_free(adc_dev->trig);
There is a devm trigger allocation function now. Using that
would make things a little cleaner.
> + free_irq(adc_dev->irq, indio_dev);
Best to use the devm interfaces for irq handling unless
there is a good reason to not do so (cleaner code ;)
> iio_device_unregister(indio_dev);
We would normally expect this function to run in exact
reverse order of the probe. That is not the case here
so please reorder things so it is.
> + iio_buffer_unregister(indio_dev);
> tiadc_channels_remove(indio_dev);
>
> step_en = get_adc_step_mask(adc_dev);
> @@ -298,10 +495,16 @@ static int tiadc_resume(struct device *dev)
>
> /* Make sure ADC is powered up */
This comment doesn't seem to me to be true any more.
> restore = tiadc_readl(adc_dev, REG_CTRL);
> - restore &= ~(CNTRLREG_POWERDOWN);
Some coments here would be good to explain the sequence that
is going on.
I'm guessing a bit, but looks like you turn off the touch screen
but leave everything else alone, then set the threshold then
reenable the touch screen before powering up the screen?
> + restore &= ~(CNTRLREG_TSCSSENB);
> tiadc_writel(adc_dev, REG_CTRL, restore);
>
> - tiadc_step_config(adc_dev);
> + tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
> + tiadc_step_config(indio_dev);
> +
> + /* Make sure ADC is powered up */
> + restore &= ~(CNTRLREG_POWERDOWN);
> + restore |= CNTRLREG_TSCSSENB;
> + tiadc_writel(adc_dev, REG_CTRL, restore);
>
> return 0;
> }
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index db1791b..a372ebf 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -46,17 +46,25 @@
> /* Step Enable */
> #define STEPENB_MASK (0x1FFFF << 0)
> #define STEPENB(val) ((val) << 0)
> +#define ENB(val) (1 << (val))
> +#define STPENB_STEPENB STEPENB(0x1FFFF)
> +#define STPENB_STEPENB_TC STEPENB(0x1FFF)
>
> /* IRQ enable */
> #define IRQENB_HW_PEN BIT(0)
> #define IRQENB_FIFO0THRES BIT(2)
> +#define IRQENB_FIFO0OVRRUN BIT(3)
> +#define IRQENB_FIFO0UNDRFLW BIT(4)
> #define IRQENB_FIFO1THRES BIT(5)
> +#define IRQENB_FIFO1OVRRUN BIT(6)
> +#define IRQENB_FIFO1UNDRFLW BIT(7)
> #define IRQENB_PENUP BIT(9)
>
> /* Step Configuration */
> #define STEPCONFIG_MODE_MASK (3 << 0)
> #define STEPCONFIG_MODE(val) ((val) << 0)
> #define STEPCONFIG_MODE_HWSYNC STEPCONFIG_MODE(2)
> +#define STEPCONFIG_MODE_SWCNT STEPCONFIG_MODE(1)
> #define STEPCONFIG_AVG_MASK (7 << 2)
> #define STEPCONFIG_AVG(val) ((val) << 2)
> #define STEPCONFIG_AVG_16 STEPCONFIG_AVG(4)
> @@ -124,6 +132,7 @@
> #define MAX_CLK_DIV 7
> #define TOTAL_STEPS 16
> #define TOTAL_CHANNELS 8
> +#define FIFO1_THRESHOLD 19
>
> /*
> * ADC runs at 3MHz, and it takes
> @@ -153,6 +162,10 @@ struct ti_tscadc_dev {
>
> /* adc device */
> struct adc_device *adc;
> +
> + /* Context save */
> + unsigned int irqstat;
Where are these used?
> + unsigned int ctrl;
> };
>
> static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>
^ permalink raw reply
* [PATCH] Input: cypress_ps2 - Remove casting the return value which is a void pointer
From: Jingoo Han @ 2013-09-09 5:37 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: 'Dmitry Torokhov', linux-input, 'Dudley Du',
'Jingoo Han'
Casting the return value which is a void pointer is redundant.
The conversion from void pointer to any other pointer type is
guaranteed by the C programming language.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/input/mouse/cypress_ps2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/cypress_ps2.c b/drivers/input/mouse/cypress_ps2.c
index f51765f..ef651cc 100644
--- a/drivers/input/mouse/cypress_ps2.c
+++ b/drivers/input/mouse/cypress_ps2.c
@@ -679,7 +679,7 @@ int cypress_init(struct psmouse *psmouse)
{
struct cytp_data *cytp;
- cytp = (struct cytp_data *)kzalloc(sizeof(struct cytp_data), GFP_KERNEL);
+ cytp = kzalloc(sizeof(struct cytp_data), GFP_KERNEL);
psmouse->private = (void *)cytp;
if (cytp == NULL)
return -ENOMEM;
--
1.7.10.4
^ permalink raw reply related
* Re: [RFC] surface-input
From: Florian Echtler @ 2013-09-09 8:25 UTC (permalink / raw)
To: Benjamin Tissoires, David Herrmann
Cc: linux-input, Henrik Rydberg, Dmitry Torokhov
In-Reply-To: <5229D7CF.4090401@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]
On 06.09.2013 15:25, Benjamin Tissoires wrote:
> Some generic comments:
> - please always inline the code in the message, it is *much* easier to review and comment it
> - please directly use a patch format: if the code is good, Dmitry can take it directly through his tree
> - add the following people for further submissions:
> * Henrik - multitouch maintainer in the kernel, and I'm sure he will be happy to see this stuff
> * Dmitry - input maintainer, the driver will go through his tree, so it's better to let him know as soon as possible of the different discussions.
> - please stick to the kernel formatting guidelines (without orders: do not use C99-style ("// ..."), do not mix tabs and spaces, stick to 80 columns, etc..). The whole documentation is in Documentation/CodingStyle, and use the script scripts/checkpatch.pl to validate most of these.
> - I don't think a separate ".h" will be accepted as the declarations will not be used outside of the driver. Just merge the header on top of you .c file.
Benjamin and David, thanks for your feedback. I will integrate this and
get back to you with a proper patch ASAP.
I have one additional question which is more about "future-proofing"
this driver. The SUR40 hardware does also provide a raw video image from
the touch sensor over another USB _endpoint_. Since it's not a different
_interface_, this can only be handled in the same driver as far as I can
tell. At some point in the future, I would like to add a V4L2 interface
to also support the video endpoint - are there any issues I should
already try to watch out for in the input part?
Thanks again, and best regards, Florian
--
SENT FROM MY DEC VT50 TERMINAL
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC] surface-input
From: David Herrmann @ 2013-09-09 9:35 UTC (permalink / raw)
To: Florian Echtler
Cc: Benjamin Tissoires, linux-input, Henrik Rydberg, Dmitry Torokhov
In-Reply-To: <522D860E.4020909@butterbrot.org>
Hi
On Mon, Sep 9, 2013 at 10:25 AM, Florian Echtler <floe@butterbrot.org> wrote:
> On 06.09.2013 15:25, Benjamin Tissoires wrote:
>> Some generic comments:
>> - please always inline the code in the message, it is *much* easier to review and comment it
>> - please directly use a patch format: if the code is good, Dmitry can take it directly through his tree
>> - add the following people for further submissions:
>> * Henrik - multitouch maintainer in the kernel, and I'm sure he will be happy to see this stuff
>> * Dmitry - input maintainer, the driver will go through his tree, so it's better to let him know as soon as possible of the different discussions.
>> - please stick to the kernel formatting guidelines (without orders: do not use C99-style ("// ..."), do not mix tabs and spaces, stick to 80 columns, etc..). The whole documentation is in Documentation/CodingStyle, and use the script scripts/checkpatch.pl to validate most of these.
>> - I don't think a separate ".h" will be accepted as the declarations will not be used outside of the driver. Just merge the header on top of you .c file.
>
> Benjamin and David, thanks for your feedback. I will integrate this and
> get back to you with a proper patch ASAP.
>
> I have one additional question which is more about "future-proofing"
> this driver. The SUR40 hardware does also provide a raw video image from
> the touch sensor over another USB _endpoint_. Since it's not a different
> _interface_, this can only be handled in the same driver as far as I can
> tell. At some point in the future, I would like to add a V4L2 interface
> to also support the video endpoint - are there any issues I should
> already try to watch out for in the input part?
Media drivers use input-devices to report input data. So no, there is
no reason to change your driver's design. Once you want
media-functionality, you simply add a v4l2 device during hid_probe()
and remove it in hid_remove(). User-space can see the relation between
both via sysfs.
It is never wrong to think about the API _now_. But as long as it's
only a single input-dev and one v4l2-dev, I'd recommend to put both
below your usb-device and you're fine.
Regarding internal driver-design, you should not bother now. Make it
work for input and make it work well.
Cheers
David
^ permalink raw reply
* [PATCH] hid-elo: some systems cannot stomach work around
From: oliver @ 2013-09-09 10:16 UTC (permalink / raw)
To: linux-usb, linux-input, jkosina, jslaby; +Cc: Oliver Neukum
From: Oliver Neukum <oneukum@suse.de>
Some systems although they have firmware class 'M', which usually
needs a work around to not crash, must not be subjected to the
work around because the work around crashes them. They cannot be
told apart by their own device descriptor, but as they are part
of compound devices can be identified by looking at their siblings.
Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
drivers/hid/hid-elo.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c
index f042a6c..64ac53e 100644
--- a/drivers/hid/hid-elo.c
+++ b/drivers/hid/hid-elo.c
@@ -181,7 +181,40 @@ fail:
*/
static bool elo_broken_firmware(struct usb_device *dev)
{
- return use_fw_quirk && le16_to_cpu(dev->descriptor.bcdDevice) == 0x10d;
+ struct usb_device *hub = dev->parent;
+ struct usb_device *child = NULL;
+ u16 fw_lvl = le16_to_cpu(dev->descriptor.bcdDevice);
+ u16 child_vid, child_pid;
+ int i;
+
+ if (!use_fw_quirk)
+ return false;
+ if (fw_lvl != 0x10d)
+ return false;
+
+ /*iterate sibling devices of the touch controller*/
+ usb_hub_for_each_child(hub, i, child) {
+ child_vid = le16_to_cpu(child->descriptor.idVendor);
+ child_pid = le16_to_cpu(child->descriptor.idProduct);
+
+ /*
+ * If one of the devices below is present attached as a sibling of
+ * the touch controller then this is a newer IBM 4820 monitor that
+ * does not need the IBM-requested workaround if fw level is
+ * 0x010d - aka 'M'.
+ * No other HW can have this combination.
+ */
+ if (child_vid==0x04b3) {
+ switch (child_pid) {
+ case 0x4676: /*4820 21x Video*/
+ case 0x4677: /*4820 51x Video*/
+ case 0x4678: /*4820 2Lx Video*/
+ case 0x4679: /*4820 5Lx Video*/
+ return false;
+ }
+ }
+ }
+ return true;
}
static int elo_probe(struct hid_device *hdev, const struct hid_device_id *id)
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH 1/7] HID: provide a helper for validating hid reports
From: Benjamin Tissoires @ 2013-09-09 12:33 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-2-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> Many drivers need to validate the characteristics of their HID report
> during initialization to avoid misusing the reports. This adds a common
> helper to perform validation of the report exisitng, the field existing,
> and the expected number of values within the field.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
>
> ---
Thanks for the v2.
I'm happy with this one.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 2/7] HID: zeroplus: validate output report details
From: Benjamin Tissoires @ 2013-09-09 12:36 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-3-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> The zeroplus HID driver was not checking the size of allocated values
> in fields it used. A HID device could send a malicious output report
> that would cause the driver to write beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [ 1442.728680] usb 1-1: New USB device found, idVendor=0c12, idProduct=0005
> ...
> [ 1466.243173] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
>
> CVE-2013-2889
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
I don't have the report descriptor available, but the patch seems ok.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 3/7] HID: sony: validate HID output report details
From: Benjamin Tissoires @ 2013-09-09 12:39 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-4-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> This driver must validate the availability of the HID output report and
> its size before it can write LED states via buzz_set_leds(). This stops
> a heap overflow that is possible if a device provides a malicious HID
> output report:
>
> [ 108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
> ...
> [ 117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
>
> CVE-2013-2890
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
The tests shows that the device is still working with the fix :)
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* HID input dealing with multiple collections?
From: Breton M. Saunders @ 2013-09-09 12:56 UTC (permalink / raw)
To: linux-input
Hello,
I've written a USB device which supports multiple input devices:
* A touch surface digitizer (following Microsoft's specification)
* A pen surface digitizer
* A mouse emulator
In windows each of these interfaces are exposed as a separate top
level collection, e.g.:
Usage_Page(Generic Desktop)
Usage(Mouse)
Collection(Application)
...
End_Collection
Usage_Page(Digitzer)
Usage(Touch_Screen)
Collection(Application)
...
End_Collection
Usage(Digitizer)
Usage(Pen)
Collection(Application)
...
End_Collection
Now this descriptor works well in Windows; all reporting channels are
exposed correctly as hid devices and mouse/multitouch/pen input all
works correctly as expected. In linux, however, this is not the case.
What I see is that if I drop out the touch/pen functionality then the
mouse support works correctly. Likewise, if I drop any pair of other
top level collections such that I have exactly one top level collection
the system works correctly.
Am I missing something here - as in: Have I made an error in my usage
of top level collections, or is linux-input's hid support incapable of
dealing with multiple top level collections?
Thanks in advance for any advice on this!
-bms20
^ permalink raw reply
* Re: [PATCH 4/7] HID: steelseries: validate output report details
From: Benjamin Tissoires @ 2013-09-09 13:02 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-5-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> steelseries HID driver to write beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [ 167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410
> ...
> [ 182.050547] BUG kmalloc-256 (Tainted: G W ): Redzone overwritten
>
> CVE-2013-2891
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-steelseries.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> index d164911..29f328f 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev,
> goto err_free;
> }
>
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) {
> + ret = -ENODEV;
> + goto err_free;
There is a problem here in case of a failure:
hid_parse() allocates a lot of memory and sets the flag
HID_STAT_PARSED, but we are leaving the probe() without clearing all
this stuff.
In hid_parse(), when a failure is detected, we call
hid_close_report(), so we also should close the report in the same way
here (but hid_close_report() is a static function).
Cheers,
Benjamin
> + }
> +
> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> if (ret) {
> hid_err(hdev, "hw start failed\n");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] update my generaltouch driver for linux by luosong
From: Jiri Kosina @ 2013-09-09 13:04 UTC (permalink / raw)
To: android; +Cc: linux-input, Henrik Rydberg, Benjamin Tissoires
In-Reply-To: <2013090912331785917915@generaltouch.com>
On Mon, 9 Sep 2013, android wrote:
> I am a software engineer from GeneralTouch Technology Co., Ltd.
>
> I want to add some driver patches to the linux kernel .
>
> I do these jobs in hid-ids.h and hid-multitouch.c
Adding Henrik and Benjamon to CC for the hid-multitouch driver.
> The main changes in hid driver are like those:
> (1)add our new products into kernel driver
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0102 0x0102
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100 0xe100
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0101 0x0101
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0106 0x0106
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A 0x010a
> (2) correct previous bug
> - MT_QUIRK_SLOT_IS_CONTACTNUMBER
> + MT_QUIRK_SLOT_IS_CONTACTID
This needs explanation / clarification in the changelog.
> the content of patch is shown below:
>
> From 5db217392e661695058606c7919be7fa6509f1e4 Mon Sep 17 00:00:00 2001
> From: luosong android@generaltouch.com
This doesn't look like a RFC-compliant from, I think.
> Date: Mon, 9 Sep 2013 02:30:10 +0800
> Subject: [PATCH] update my generaltouch driver for linux by luosong
Please insert changelog (description of the changes) and Signed-off-by:
line here, as documented in Documentation/SubmittingPatches
> ---
> drivers/hid/hid-ids.h | 5 +++++
> drivers/hid/hid-multitouch.c | 19 +++++++++++++++++--
> 2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index ffe4c7a..ca78f09 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -332,6 +332,11 @@
> #define USB_VENDOR_ID_GENERAL_TOUCH 0x0dfc
> #define USB_DEVICE_ID_GENERAL_TOUCH_WIN7_TWOFINGERS 0x0003
> #define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PWT_TENFINGERS 0x0100
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0102 0x0102
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100 0xe100
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0101 0x0101
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0106 0x0106
> +#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A 0x010a
>
> #define USB_VENDOR_ID_GLAB 0x06c2
> #define USB_DEVICE_ID_4_PHIDGETSERVO_30 0x0038
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index cb0e361..9558dde 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -244,12 +244,12 @@ static struct mt_class mt_classes[] = {
> { .name = MT_CLS_GENERALTOUCH_TWOFINGERS,
> .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
> MT_QUIRK_VALID_IS_INRANGE |
> - MT_QUIRK_SLOT_IS_CONTACTNUMBER,
> + MT_QUIRK_SLOT_IS_CONTACTID,
> .maxcontacts = 2
> },
> { .name = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
> - MT_QUIRK_SLOT_IS_CONTACTNUMBER
> + MT_QUIRK_SLOT_IS_CONTACTID
> },
>
> { .name = MT_CLS_FLATFROG,
> @@ -1191,6 +1191,21 @@ static const struct hid_device_id mt_devices[] = {
> { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PWT_TENFINGERS) },
> + { .driver_data = MT_CLS_GENERALTOUCH_TWOFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0101) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0102) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_0106) },
> + { .driver_data = MT_CLS_GENERALTOUCH_PWT_TENFINGERS,
> + MT_USB_DEVICE(USB_VENDOR_ID_GENERAL_TOUCH,
> + USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A) },
Your mail client seems to be whitespace-corrupting patches (it ate the
tabs at least).
Could you please fix all the above and resubmit?
Thanks a lot,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Benjamin Tissoires @ 2013-09-09 13:12 UTC (permalink / raw)
To: Breton M. Saunders; +Cc: linux-input
In-Reply-To: <522DC594.2000305@brettnet.eu>
Hi,
On Mon, Sep 9, 2013 at 2:56 PM, Breton M. Saunders <bms20@brettnet.eu> wrote:
> Hello,
>
> I've written a USB device which supports multiple input devices:
> * A touch surface digitizer (following Microsoft's specification)
> * A pen surface digitizer
> * A mouse emulator
>
> In windows each of these interfaces are exposed as a separate top level
> collection, e.g.:
> Usage_Page(Generic Desktop)
> Usage(Mouse)
> Collection(Application)
> ...
> End_Collection
> Usage_Page(Digitzer)
> Usage(Touch_Screen)
> Collection(Application)
> ...
> End_Collection
> Usage(Digitizer)
> Usage(Pen)
> Collection(Application)
> ...
> End_Collection
>
> Now this descriptor works well in Windows; all reporting channels are
> exposed correctly as hid devices and mouse/multitouch/pen input all works
> correctly as expected. In linux, however, this is not the case. What I see
> is that if I drop out the touch/pen functionality then the mouse support
> works correctly. Likewise, if I drop any pair of other top level
> collections such that I have exactly one top level collection the system
> works correctly.
>
> Am I missing something here - as in: Have I made an error in my usage of
> top level collections, or is linux-input's hid support incapable of dealing
> with multiple top level collections?
If it works under Windows, there are huge chances that your report
descriptor is good.
However, under Linux, we use the report IDs to split (if required) the
different inputs.
The good thing for you is that if I understand correctly, the device
you are talking about should be handled through hid-multitouch. Since
kernel v3.10, we have introduced the support of devices showing both a
digitizer and a pen (they will show up as two input devices).
I would encourage you to test a 3.10 kernel, and/or send me some
traces of your devices by using hid-replay[1]. This way I will be able
to test your device on my laptop, and eventually figure out any
problem.
Cheers,
Benjamin
[1] http://bentiss.github.io/hid-replay-docs/
^ permalink raw reply
* Re: [PATCH 5/7] HID: LG: validate HID output report details
From: Benjamin Tissoires @ 2013-09-09 13:22 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-6-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> lg, lg3, and lg4 HID drivers to write beyond the output report allocation
> during an event, causing a heap overflow:
>
> [ 325.245240] usb 1-1: New USB device found, idVendor=046d, idProduct=c287
> ...
> [ 414.518960] BUG kmalloc-4096 (Not tainted): Redzone overwritten
>
> Additionally, while lg2 did correctly validate the report details, it was
> cleaned up and shortened.
>
> CVE-2013-2893
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
To me, the patch looks good.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 6/7] HID: lenovo-tpkbd: validate output report details
From: Benjamin Tissoires @ 2013-09-09 13:28 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
In-Reply-To: <1378312645-27736-7-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> lenovo-tpkbd HID driver to write just beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [ 76.109807] usb 1-1: New USB device found, idVendor=17ef, idProduct=6009
> ...
> [ 80.462540] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
>
> CVE-2013-2894
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
As mentioned by Josh on the previous thread, this patch triggers a bug
for the users:
https://bugzilla.redhat.com/show_bug.cgi?id=1003998
I am writing the comments I have given the bug:
> drivers/hid/hid-lenovo-tpkbd.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
> index 07837f5..f458e76 100644
> --- a/drivers/hid/hid-lenovo-tpkbd.c
> +++ b/drivers/hid/hid-lenovo-tpkbd.c
> @@ -339,7 +339,15 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
> struct tpkbd_data_pointer *data_pointer;
> size_t name_sz = strlen(dev_name(dev)) + 16;
> char *name_mute, *name_micmute;
> - int ret;
> + int i, ret;
> +
> + /* Validate required reports. */
> + for (i = 0; i < 4; i++) {
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 4, i, 1))
The report descriptor shows that the report ID 4 is a FEATURE, not an OUTPUT.
> + return -ENODEV;
There is a problem in the probe() function when tpkbd_probe_tp()
fails: it directly bails out instead of calling hid_hw_stop(), thus,
the input devices are still there while nobody has a pointer to them.
This way, ODEBUG is triggered. But this should be address in an other
patch I would say.
Cheers,
Benjamin
> + }
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
> + return -ENODEV;
>
> if (sysfs_create_group(&hdev->dev.kobj,
> &tpkbd_attr_group_pointer)) {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/7] HID: logitech-dj: validate output report details
From: Benjamin Tissoires @ 2013-09-09 13:44 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-input, Jiri Kosina, Henrik Rydberg
In-Reply-To: <1378312645-27736-8-git-send-email-keescook@chromium.org>
On 04/09/13 18:37, Kees Cook wrote:
> A HID device could send a malicious output report that would cause the
> logitech-dj HID driver to leak kernel memory contents to the device, or
> trigger a NULL dereference during initialization:
>
> [ 304.424553] usb 1-1: New USB device found, idVendor=046d, idProduct=c52b
> ...
> [ 304.780467] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
> [ 304.781409] IP: [<ffffffff815d50aa>] logi_dj_recv_send_report.isra.11+0x1a/0x90
>
> CVE-2013-2895
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-logitech-dj.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index cd33084..404f2d0 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -461,7 +461,7 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> struct hid_report *report;
> struct hid_report_enum *output_report_enum;
> u8 *data = (u8 *)(&dj_report->device_index);
> - int i;
> + unsigned int i, length;
>
> output_report_enum = &hdev->report_enum[HID_OUTPUT_REPORT];
> report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
> @@ -471,7 +471,9 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
> return -ENODEV;
> }
>
> - for (i = 0; i < report->field[0]->report_count; i++)
> + length = min_t(size_t, sizeof(*dj_report) - 1,
> + report->field[0]->report_count);
> + for (i = 0; i < length; i++)
I would rather simply do:
for (i = 0; i < DJREPORT_SHORT_LENGTH - 1; i++)
> report->field[0]->value[i] = data[i];
>
> hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> @@ -783,6 +785,12 @@ static int logi_dj_probe(struct hid_device *hdev,
> goto hid_parse_fail;
> }
>
> + if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
> + 0, 3)) {
and here:
if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
0, DJREPORT_SHORT_LENGTH - 1)) {
3 seems to come from nowhere, and is also wrong because in logi_dj_recv_switch_to_dj_mode(), we write 4 bytes.
Besides this small nitpick, there is the same problem that I spotted in hid-steelseries, in case of a failure, the hid device is not cleaned correctly.
Cheers,
Benjamin
> + retval = -ENODEV;
> + goto hid_parse_fail;
> + }
> +
> /* Starts the usb device and connects to upper interfaces hiddev and
> * hidraw */
> retval = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>
^ permalink raw reply
* Re: [PATCH v2 0/7] HID: validate report details
From: Benjamin Tissoires @ 2013-09-09 13:48 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg
In-Reply-To: <1378312645-27736-1-git-send-email-keescook@chromium.org>
On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> These patches introduce a validation function for HID devices that do
> direct report value accesses, solving a number of heap smashing flaws.
>
> This version changes to using an field-index-based checker for the new
> "hid_validate_values()" which requires callers to loop across fields if
> they use more than one field.
I am globally happy with the patch series.
I have some concerns about patches 4 6 and 7, but the other can be
applied right now.
Kees, if you want to switch to something else, I can handle the v3 for
these three patches: I have some logitech-dj devices and a tester for
the lenovo one.
Cheers,
Benjamin
>
> -Kees
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 11/14] HID: multitouch: validate feature report details
From: Benjamin Tissoires @ 2013-09-09 13:50 UTC (permalink / raw)
To: Kees Cook; +Cc: Benjamin Tissoires, Jiri Kosina, linux-input, Henrik Rydberg
In-Reply-To: <CAGXu5j+LHg-kFBXRzXfpLmztHjVCn6jiKkX5w1x3-a4is_RLqw@mail.gmail.com>
On Wed, Sep 4, 2013 at 6:31 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Aug 30, 2013 at 11:27 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Aug 30, 2013 at 8:27 AM, Benjamin Tissoires
>> <benjamin.tissoires@gmail.com> wrote:
>>> On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires
>>>> <benjamin.tissoires@redhat.com> wrote:
>>>>> Hi Kees,
>>>>>
>>>>> I would be curious to have the HID report descriptors (maybe off list)
>>>>> to understand how things can be that bad.
>>>>
>>>> Certainly! I'll send them your way. I did have to get pretty creative
>>>> to tickle these conditions.
>>>>
>>>>> On overall, I'd prefer all those checks to be in hid-core so that we
>>>>> have the guarantee that we don't have to open a new CVE each time a
>>>>> specific hid driver do not check for these ranges.
>>>>
>>>> I pondered doing this, but it seemed like something that needed wider
>>>> discussion, so I thought I'd start with just the dump of fixes. It
>>>> seems like the entire HID report interface should use access functions
>>>> to enforce range checking -- perhaps further enforced by making the
>>>> structure opaque to the drivers.
>>>
>>> The problem with access functions with range checking is when they are
>>> used at each input report. This will overload the kernel processing
>>> for static checks.
>>>
>>>>
>>>>> More specific comments inlined:
>>>>>
>>>>> On 28/08/13 22:31, Jiri Kosina wrote:
>>>>>> From: Kees Cook <keescook@chromium.org>
>>>>>>
>>>>>> When working on report indexes, always validate that they are in bounds.
>>>>>> Without this, a HID device could report a malicious feature report that
>>>>>> could trick the driver into a heap overflow:
>>>>>>
>>>>>> [ 634.885003] usb 1-1: New USB device found, idVendor=0596, idProduct=0500
>>>>>> ...
>>>>>> [ 676.469629] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
>>>>>>
>>>>>> CVE-2013-2897
>>>>>>
>>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>>> Cc: stable@kernel.org
>>>>>> ---
>>>>>> drivers/hid/hid-multitouch.c | 25 ++++++++++++++++++++-----
>>>>>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>>>>>> index cb0e361..2aa275e 100644
>>>>>> --- a/drivers/hid/hid-multitouch.c
>>>>>> +++ b/drivers/hid/hid-multitouch.c
>>>>>> @@ -330,9 +330,18 @@ static void mt_feature_mapping(struct hid_device *hdev,
>>>>>> break;
>>>>>> }
>>>>>> }
>>>>>> + /* Ignore if value index is out of bounds. */
>>>>>> + if (td->inputmode_index < 0 ||
>>>>>
>>>>> td->inputmode_index can not be less than 0
>>>>
>>>> Well, it certainly _shouldn't_ be less than zero. :) However, it is
>>>> defined as s8, and gets set from an int:
>>>>
>>>> for (i=0; i < field->maxusage; i++) {
>>>> if (field->usage[i].hid == usage->hid) {
>>>> td->inputmode_index = i;
>>>> break;
>>>> }
>>>> }
>>>>
>>>> Both "i" and "maxusage" are int, and I can generate a large maxusage
>>>> and usage array where the first matching hid equality happens when i
>>>> is >127, causing inputmode_index to wrap.
>>>
>>> ouch. Sorry. This piece of code (the current code) is junk. We should
>>> switch to usage->usage_index and change from __s8 to __s16 the
>>> declaration ASAP.
>>>
>>>>
>>>>>> + td->inputmode_index >= field->report_count) {
>>>>>
>>>>> if this is really required, we could just change the for loop above to
>>>>> go from 0 to field->report_count instead.
>>>>
>>>> That's certainly true, but since usage count and report count are not
>>>> directly associated, I don't know if there are devices that will freak
>>>> out with this restriction.
>>>
>>> Actually, I just again another long time understanding all the mess of
>>> having usage count and report count different in hid-core.
>>>
>>> I think it is a bug at some point (but it looks like I tend to be
>>> wrong on a lot of things recently :-P ), because when you look at the
>>> actual code of hid_input_field() in hid-core.c, we never go further
>>> than field->report_count when dealing with input report.
>>> So my guess is that we are declaring extra usages that are never used
>>> to parse incoming data.
>>>
>>>>
>>>>> However, I think we could just rely on usage->usage_index to get the
>>>>> actual index.
>>>>> I'll do more tests today and tell you later.
>>>>>
>>>>>> + dev_err(&hdev->dev, "HID_DG_INPUTMODE out of range\n");
>>>>>> + td->inputmode = -1;
>>>>>> + }
>>>>>>
>>>>>> break;
>>>>>> case HID_DG_CONTACTMAX:
>>>>>> + /* Ignore if value count is out of bounds. */
>>>>>> + if (field->report_count < 1)
>>>>>> + break;
>>>>>
>>>>> If you can trigger this, I would say that the fix should be in hid-core,
>>>>> not in every driver. A null report_count should not be allowed by the
>>>>> HID protocol.
>>>>
>>>> Again, I have no problem with that idea, but there seem to be lots of
>>>> general cases where this may not be possible (i.e. a HID with 0 OUTPUT
>>>> or FEATURE reports). I didn't see a sensible way to approach this in
>>>> core without declaring a "I require $foo many OUTPUT reports, $bar
>>>> many INPUT, and $baz many FEATURE" for each driver. I instead opted
>>>> for the report validation function instead.
>>>>
>>>
>>> I think we can just add this check in both hidinput_configure_usage()
>>> and report_features() (both in hid-input.c) so that we don't call the
>>> *_mapping() callbacks with non sense fields.
>
> Can you suggest a patch for this? (And please include reference to
> CVE-2013-2897 in the commit.)
Sure, I'll do that.
Cheers,
Benjamin
>
> I'll send a v2 of the rest of the series.
>
> Thanks!
>
> -Kees
>
>>> This way, the specific hid drivers will know only the correct fields,
>>> and don't need to check this. So patches 9, 11 and 13 can be amended.
>>>
>>> It looks to me that you mismatched field->report_count and the actual
>>> report_count in your answer: field->report_count is the number of
>>> consecutive fields of field->report_size that are present for the
>>> parsing of the report. It has nothing to do with the total report
>>> count.
>>>
>>>>>> td->maxcontact_report_id = field->report->id;
>>>>>> td->maxcontacts = field->value[0];
>>>>>> if (!td->maxcontacts &&
>>>>>> @@ -743,15 +752,21 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
>>>>>> unsigned count;
>>>>>> int r, n;
>>>>>>
>>>>>> + if (report->maxfield == 0)
>>>>>> + return;
>>>>>> +
>>>>>> /*
>>>>>> * Includes multi-packet support where subsequent
>>>>>> * packets are sent with zero contactcount.
>>>>>> */
>>>>>> - if (td->cc_index >= 0) {
>>>>>> - struct hid_field *field = report->field[td->cc_index];
>>>>>> - int value = field->value[td->cc_value_index];
>>>>>> - if (value)
>>>>>> - td->num_expected = value;
>>>>>> + if (td->cc_index >= 0 && td->cc_index < report->maxfield) {
>>>>>> + field = report->field[td->cc_index];
>>>>>
>>>>> looks like we previously overwrote the definition of field :(
>>>>>
>>>>>> + if (td->cc_value_index >= 0 &&
>>>>>> + td->cc_value_index < field->report_count) {
>>>>>> + int value = field->value[td->cc_value_index];
>>>>>> + if (value)
>>>>>> + td->num_expected = value;
>>>>>> + }
>>>>>
>>>>> I can not see why td->cc_index and td->cc_value_index could have bad
>>>>> values. They are initially created by hid-core, and are not provided by
>>>>> the device.
>>>>> Anyway, if you are able to produce bad values with them, I'd rather have
>>>>> those checks during the assignment of td->cc_index (in
>>>>> mt_touch_input_mapping()), instead of checking them for each report.
>>>>
>>>> Well, the problem comes again from there not being a hard link between
>>>> the usage array and the value array:
>>>>
>>>> td->cc_value_index = usage->usage_index;
>>>> ...
>>>> int value = field->value[td->cc_value_index];
>>>>
>>>> And all of this would be irrelevant if there was an access function
>>>> for field->value[].
>>>
>>> True, with the current implementation, td->cc_value_index can be
>>> greater than field->report_count. Still, moving this check in
>>> mt_touch_input_mapping() will allow us to make it only once.
>>>
>>>>
>>>> Thanks for the review!
>>>
>>> you are welcome :)
>>
>> Okay, so, where does the whole patch series stand? It seems like the
>> checks are all worth adding; and my fixes are, I think, "minimal" so
>> having them go into the tree (so that they land in stable) seems like
>> a good idea. The work beyond that could be done on top of the existing
>> patches? There seems to be a lot of redesign ideas, but those probably
>> aren't so great for stable, and I'm probably not the best person to
>> write them, since everything I know about HID code I learned two weeks
>> ago. :)
>>
>> Given the 12 flaws, what do you see as the best way forward?
>
> --
> Kees Cook
> Chrome OS Security
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Breton M. Saunders @ 2013-09-09 14:01 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <CAN+gG=ELptZsD37P9XMAgbLCFt-yZVbgmbGrk+mLvT6G2raKLQ@mail.gmail.com>
On 09/09/13 14:12, Benjamin Tissoires wrote:
> Hi,
>
> On Mon, Sep 9, 2013 at 2:56 PM, Breton M. Saunders <bms20@brettnet.eu> wrote:
>> Hello,
>>
>> I've written a USB device which supports multiple input devices:
>> * A touch surface digitizer (following Microsoft's specification)
>> * A pen surface digitizer
>> * A mouse emulator
>>
>> In windows each of these interfaces are exposed as a separate top level
>> collection, e.g.:
>> Usage_Page(Generic Desktop)
>> Usage(Mouse)
>> Collection(Application)
>> ...
>> End_Collection
>> Usage_Page(Digitzer)
>> Usage(Touch_Screen)
>> Collection(Application)
>> ...
>> End_Collection
>> Usage(Digitizer)
>> Usage(Pen)
>> Collection(Application)
>> ...
>> End_Collection
>>
>> Now this descriptor works well in Windows; all reporting channels are
>> exposed correctly as hid devices and mouse/multitouch/pen input all works
>> correctly as expected. In linux, however, this is not the case. What I see
>> is that if I drop out the touch/pen functionality then the mouse support
>> works correctly. Likewise, if I drop any pair of other top level
>> collections such that I have exactly one top level collection the system
>> works correctly.
>>
>> Am I missing something here - as in: Have I made an error in my usage of
>> top level collections, or is linux-input's hid support incapable of dealing
>> with multiple top level collections?
> If it works under Windows, there are huge chances that your report
> descriptor is good.
>
> However, under Linux, we use the report IDs to split (if required) the
> different inputs.
> The good thing for you is that if I understand correctly, the device
> you are talking about should be handled through hid-multitouch. Since
> kernel v3.10, we have introduced the support of devices showing both a
> digitizer and a pen (they will show up as two input devices).
>
> I would encourage you to test a 3.10 kernel, and/or send me some
> traces of your devices by using hid-replay[1]. This way I will be able
> to test your device on my laptop, and eventually figure out any
> problem.
>
> Cheers,
Hi Benjamin,
Thanks for the quick reply.
3.10 is a no-go for me; I need to get this working on a vanilla (or
upgraded) version of Ubuntu 12.04; so something more like kernel version
3.2. (Unfortunately, its what people are using in the field).
I will, however, install a more modern kernel to test against; esp.
since I note your HID-replay tool requires 3.6 to test with.
Would the correct approach here be to implement a composite USB
device and have separate USB interfaces for mouse, touch and pen for
compatibility with older kernel revisions?
Cheers,
-Brett
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Benjamin Tissoires @ 2013-09-09 14:16 UTC (permalink / raw)
To: Breton M. Saunders; +Cc: linux-input
In-Reply-To: <522DD4AA.3020501@brettnet.eu>
On Mon, Sep 9, 2013 at 4:01 PM, Breton M. Saunders <bms20@brettnet.eu> wrote:
> On 09/09/13 14:12, Benjamin Tissoires wrote:
>>
>> Hi,
>>
>> On Mon, Sep 9, 2013 at 2:56 PM, Breton M. Saunders <bms20@brettnet.eu>
>> wrote:
>>>
>>> Hello,
>>>
>>> I've written a USB device which supports multiple input devices:
>>> * A touch surface digitizer (following Microsoft's specification)
>>> * A pen surface digitizer
>>> * A mouse emulator
>>>
>>> In windows each of these interfaces are exposed as a separate top
>>> level
>>> collection, e.g.:
>>> Usage_Page(Generic Desktop)
>>> Usage(Mouse)
>>> Collection(Application)
>>> ...
>>> End_Collection
>>> Usage_Page(Digitzer)
>>> Usage(Touch_Screen)
>>> Collection(Application)
>>> ...
>>> End_Collection
>>> Usage(Digitizer)
>>> Usage(Pen)
>>> Collection(Application)
>>> ...
>>> End_Collection
>>>
>>> Now this descriptor works well in Windows; all reporting channels are
>>> exposed correctly as hid devices and mouse/multitouch/pen input all works
>>> correctly as expected. In linux, however, this is not the case. What I
>>> see
>>> is that if I drop out the touch/pen functionality then the mouse support
>>> works correctly. Likewise, if I drop any pair of other top level
>>> collections such that I have exactly one top level collection the system
>>> works correctly.
>>>
>>> Am I missing something here - as in: Have I made an error in my usage
>>> of
>>> top level collections, or is linux-input's hid support incapable of
>>> dealing
>>> with multiple top level collections?
>>
>> If it works under Windows, there are huge chances that your report
>> descriptor is good.
>>
>> However, under Linux, we use the report IDs to split (if required) the
>> different inputs.
>> The good thing for you is that if I understand correctly, the device
>> you are talking about should be handled through hid-multitouch. Since
>> kernel v3.10, we have introduced the support of devices showing both a
>> digitizer and a pen (they will show up as two input devices).
>>
>> I would encourage you to test a 3.10 kernel, and/or send me some
>> traces of your devices by using hid-replay[1]. This way I will be able
>> to test your device on my laptop, and eventually figure out any
>> problem.
>>
>> Cheers,
>
> Hi Benjamin,
>
> Thanks for the quick reply.
>
> 3.10 is a no-go for me; I need to get this working on a vanilla (or
> upgraded) version of Ubuntu 12.04; so something more like kernel version
> 3.2. (Unfortunately, its what people are using in the field).
You can still try to use my backport available here:
https://github.com/bentiss/hid-multitouch
it _should_ work on a 3.2. You only have to add your VID/PID in
hid-multitouch to make it work.
>
> I will, however, install a more modern kernel to test against; esp. since
> I note your HID-replay tool requires 3.6 to test with.
well, it requires the module uhid, which is available in 3.6, but also
in my backport :)
run "modprobe uhid_compat" and run hid-replay against the
/dev/hidraw-compatN device.
Anyway, if this is too much of a pain, just send me the report
descriptor in hexadecimal (and some hexadecimal captures of your
events), I'll do the formatting myself.
>
> Would the correct approach here be to implement a composite USB device and
> have separate USB interfaces for mouse, touch and pen for compatibility with
> older kernel revisions?
I would say no because I don't know what would be the behavior of
Windows. But if you don't care about Windows and if this solution
works, it's entirely up to you.
Cheers,
Benjamin
^ permalink raw reply
* Re: HID input dealing with multiple collections?
From: Breton M. Saunders @ 2013-09-09 15:40 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
In-Reply-To: <CAN+gG=F9OmeXM_s_MX1_q+H=hAwZB518Lx_6FZd42Z8PX=SpJw@mail.gmail.com>
HI Benjamin,
> You can still try to use my backport available here:
> https://github.com/bentiss/hid-multitouch
Great - thanks for that! It may be a solution.
>
> it _should_ work on a 3.2. You only have to add your VID/PID in
> hid-multitouch to make it work.
Unfortunately not. It seems that writes on the device to the interrupt
endpoint never complete; I haven't had the USB analyzer on this yet, but
I suspect that the host running kernel 3.2 is not polling the endpoint.
I did upgrade my Mint13 box to Ubuntu's latest kernel 3.11; and the
multitouch support looks pretty good. It worked out of the box.
However, the mouse channel support isn't working - if I cat /dev/mouse1
or /dev/mouse2 I see no data coming over when I report mouse events from
my device.
>
>> I will, however, install a more modern kernel to test against; esp. since
>> I note your HID-replay tool requires 3.6 to test with.
> well, it requires the module uhid, which is available in 3.6, but also
> in my backport :)
> run "modprobe uhid_compat" and run hid-replay against the
> /dev/hidraw-compatN device.
>
> Anyway, if this is too much of a pain, just send me the report
> descriptor in hexadecimal (and some hexadecimal captures of your
> events), I'll do the formatting myself.
Let me check if I can do this; I may get permission to open up the
descriptors too.
>> Would the correct approach here be to implement a composite USB device and
>> have separate USB interfaces for mouse, touch and pen for compatibility with
>> older kernel revisions?
> I would say no because I don't know what would be the behavior of
> Windows. But if you don't care about Windows and if this solution
> works, it's entirely up to you.
>
I may have to resort to that approach. I believe Microsoft added the
mouse top level collection to support windows xp; so it may also just
work out of the box if a separate mouse appears on the same interface.
Cheers,
-Brett
^ permalink raw reply
* [BUG?] HID: uhid: add devname module alias
From: Tom Gundersen @ 2013-09-09 15:43 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Jiri Kosina, David Herrmann, Kay Sievers,
linux-input@vger.kernel.org, LKML
Hi Marcel,
The above commit (60cbd53 in mainline) doesn't appear to work for me.
I.e., depmod does not create an entry in modules.devname and hence no
device node is created on boot.
If I understand correctly, you'd also need to create the correct
"char-major-<MAJOR>-<MINOR>" alias (which I don't think you can do as
long as you are using MISC_DYNAMIC_MINOR).
Or am I missing something?
Cheers,
Tom
^ permalink raw reply
* Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC
From: Dmitry Torokhov @ 2013-09-09 16:12 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Zubair Lutfullah, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
bigeasy-hfZtesqFncYOwBW4kG4KsQ,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <522C5F96.20001-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Sun, Sep 08, 2013 at 12:29:26PM +0100, Jonathan Cameron wrote:
> On 09/01/13 12:17, Zubair Lutfullah wrote:
> > Enable shared IRQ to allow ADC to share IRQ line from
> > parent MFD core. Only FIFO0 IRQs are for TSC and handled
> > on the TSC side.
> >
> > Step mask would be updated from cached variable only previously.
> > In rare cases when both TSC and ADC are used, the cached
> > variable gets mixed up.
> > The step mask is written with the required mask every time.
> >
> > Rachna Patil (TI) laid ground work for shared IRQ.
> >
> > Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Whilst I would have prefered an mfd under these drivers and elegant handling
> of the interrupts, I guess if this works it is at least not terribly invasive.
>
> However, this does need an Ack from Dmitry before I can take it (or for
> Dmitry to take it himself?)
I completely agree with Jonathan, more elegant handling would be nice
but current one will do for now.
Since most of the work on the driver is going through IIO tree I think
it would make sense for this patch to go through it as well.
Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: ALPS Touchpad- Improve the performance of alps v5-protocol's touchpad
From: Dmitry Torokhov @ 2013-09-09 16:26 UTC (permalink / raw)
To: yunkang.tang@cn.alps.com; +Cc: linux-input@vger.kernel.org
In-Reply-To: <89B48266C4EEC447B3A3A031C8DE21870263AF88@SIXPRD0410MB394.apcprd04.prod.outlook.com>
Hi Tommy,
On Thu, Sep 05, 2013 at 05:50:48AM +0000, yunkang.tang@cn.alps.com wrote:
> <Change list>
> - Calculate the device's dimension in alps_identify().
> - Change the logic of packet decoding.
> - Add the new data process logic.
It appears that your mailer damaged the patch converting all tabs to
single spaces. Please consider using an alternative MTA (mutt, or maybe
git send-email) to post your patches.
It also apperas that there are several changes (support for never
hardware, tweaks to the older protocols) going rolled into a single
patch, would it be possible to split them into separate patches,
please?
> - Change the dev2's name from "PS/2 Mouse" to "ALPS Touchpad".
Hmm, the secondary device is never a touchpad but either trackpoint (for
confirmed dualpoint devices) or "something" we do not quite know what.
Why are you marking it as a touchpad?
Also, you forgot to add your "Signed-off-by: ..." as per
Documentation/SubmittingPatches
Thanks!
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] Input: ALPS Touchpad- Improve the performance of alps v5-protocol's touchpad
From: Dmitry Torokhov @ 2013-09-09 16:27 UTC (permalink / raw)
To: yunkang.tang@cn.alps.com; +Cc: linux-input@vger.kernel.org
In-Reply-To: <89B48266C4EEC447B3A3A031C8DE21870263AF93@SIXPRD0410MB394.apcprd04.prod.outlook.com>
Hi Tommy,
On Thu, Sep 05, 2013 at 05:50:55AM +0000, yunkang.tang@cn.alps.com wrote:
> <Change list>
> - Add the macro definition for v5 device.
>
> --- linux-3.11/drivers/input/mouse/alps.h.orig 2013-09-04 19:51:33.837135870 +0800
> +++ linux-3.11/drivers/input/mouse/alps.h 2013-09-05 21:01:57.074314890 +0800
> @@ -18,6 +18,10 @@
> #define ALPS_PROTO_V4 4
> #define ALPS_PROTO_V5 5
>
> +#define DOLPHIN_COUNT_PER_ELECTRODE 64
> +#define DOLPHIN_PROFILE_XOFFSET 8 /* [DOLPHIN] The number of the x-electrode offset value */
> +#define DOLPHIN_PROFILE_YOFFSET 1 /* [DOLPHIN] The number of the y-electrode offset value */
> +
> /**
> * struct alps_model_info - touchpad ID table
> * @signature: E7 response string to match.
> @@ -69,7 +73,7 @@ struct alps_nibble_commands {
> * @y: Y position for ST.
> * @z: Z position for ST.
> * @first_mp: Packet is the first of a multi-packet report.
> - * @is_mp: Packet is part of a multi-packet report.
> + * @is_mp: Packet is the last of a multi-packet report.
> * @left: Left touchpad button is active.
> * @right: Right touchpad button is active.
> * @middle: Middle touchpad button is active.
> @@ -145,7 +149,7 @@ struct alps_data {
>
> int (*hw_init)(struct psmouse *psmouse);
> void (*process_packet)(struct psmouse *psmouse);
> - void (*decode_fields)(struct alps_fields *f, unsigned char *p);
> + void (*decode_fields)(struct alps_fields *f, unsigned char *p, struct psmouse *psmouse);
> void (*set_abs_params)(struct alps_data *priv, struct input_dev *dev1);
>
> int prev_fin;
Please do not separate changes in header files form changes in .c files
because patches need to be complete. If I were to apply this patch alone
it would break the compilation.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] HID: uhid: allocate static minor
From: David Herrmann @ 2013-09-09 16:33 UTC (permalink / raw)
To: linux-input
Cc: Tom Gundersen, Kay Sievers, Marcel Holtmann, linux-kernel,
David Herrmann
udev has this nice feature of creating "dead" /dev/<node> device-nodes if
it finds a devnode:<node> modalias. Once the node is accessed, the kernel
automatically loads the module that provides the node. However, this
requires udev to know the major:minor code to use for the node. This
feature was introduced by:
commit 578454ff7eab61d13a26b568f99a89a2c9edc881
Author: Kay Sievers <kay.sievers@vrfy.org>
Date: Thu May 20 18:07:20 2010 +0200
driver core: add devname module aliases to allow module on-demand auto-loading
However, uhid uses dynamic minor numbers so this doesn't actually work. We
need to load uhid to know which minor it's going to use.
Hence, allocate a static minor (just like uinput does) and we're good
to go.
Reported-by: Tom Gundersen <teg@jklm.no>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 3 ++-
include/linux/miscdevice.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 5bf2fb7..93b00d7 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -615,7 +615,7 @@ static const struct file_operations uhid_fops = {
static struct miscdevice uhid_misc = {
.fops = &uhid_fops,
- .minor = MISC_DYNAMIC_MINOR,
+ .minor = UHID_MINOR,
.name = UHID_NAME,
};
@@ -634,4 +634,5 @@ module_exit(uhid_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
MODULE_DESCRIPTION("User-space I/O driver support for HID subsystem");
+MODULE_ALIAS_MISCDEV(UHID_MINOR);
MODULE_ALIAS("devname:" UHID_NAME);
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index 09c2300..cb35835 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -45,6 +45,7 @@
#define MAPPER_CTRL_MINOR 236
#define LOOP_CTRL_MINOR 237
#define VHOST_NET_MINOR 238
+#define UHID_MINOR 239
#define MISC_DYNAMIC_MINOR 255
struct device;
--
1.8.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox