* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-17 11:28 UTC (permalink / raw)
To: Arnd Bergmann, Lars-Peter Clausen
Cc: Guenter Roeck, Richard Weinberger, Greg Kroah-Hartman,
dmitry.torokhov, linux-iio, Benjamin Herrenschmidt, teg,
Thierry Reding, Lennox Wu, Marek Vasut, Liqin Chen, msalter,
linux-pwm, devel, linux-watchdog, linux-input,
linux-kernel@vger.kernel.org, knaack.h, Martin Schwidefsky,
Mischa.Jonker, jic23, Geert Uytterhoeven, cmetcalf
In-Reply-To: <6485666.LVFO2YCEQx@wuerfel>
On 07/17/2014 06:48 PM, Arnd Bergmann wrote:
> On Thursday 17 July 2014 12:40:25 Lars-Peter Clausen wrote:
>> On 07/17/2014 11:20 AM, Arnd Bergmann wrote:
>>> On Thursday 17 July 2014 09:27:58 Chen Gang wrote:
>>>> gfp_t gfp_mask, unsigned int order);
>>>> extern void devm_free_pages(struct device *dev, unsigned long addr);
>>>>
>>>> +#ifdef CONFIG_HAS_IOMEM
>>>> void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
>>>> +#elif defined(CONFIG_COMPILE_TEST)
>>>> +static inline void __iomem *devm_ioremap_resource(struct device *dev,
>>>> + struct resource *res)
>>>> +{
>>>> + pr_warn("no hardware io memory, only for COMPILE_TEST\n");
>>>> + return (__force void __iomem *)ERR_PTR(-ENXIO);
>>>> +}
>>>> +#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */
>>>>
>>>> /* allows to add/remove a custom action to devres stack */
>>>
>>> To be honest, I think it's a bad idea to introduce wrappers functions
>>> that are only available when CONFIG_COMPILE_TEST is set.
>>>
>>> COMPILE_TEST is a great tool in general, but it has its limits.
>>> In particular, the case for !CONFIG_IOMEM is completely obscure
>>> and we won't find any bugs by allowing more drivers to be built
>>> in those configurations, but attempting to do it would cause
>>> endless churn by changing each instance of 'depends on HAS_IOMEM'
>>> to 'depends on HAS_IOMEM || COMPILE_TEST'.
>>
>> The point of this exercise is that we do not have to replace a good chunk of
>> 'depends on COMPILE_TEST' with 'depends on COMPILE_TEST && HAS_IOMEM'
>
> Ok, I see.
>
>> E.g. the typical Kconfig entry for your random SoC peripheral driver looks like
>>
>> config ARCH_FOOBAR_DRIVER
>> depends on ARCH_FOOBAR || COMPILE_TEST
>> ...
>>
>> Now when COMPILE_TEST is not set there is a implicit dependency on HAS_IOMEM
>> since the architecture will provide it. If COMPILE_TEST is selected the
>> driver will also be build-able on architectures that do no have HAS_IOMEM
>> and hence linking the driver fails. One way to fix this is of course to
>> replace the COMPILE_TEST with (COMPILE_TEST && HAS_IOMEM). But this is very
>> often overlooked and only noticed later on when somebody actually builds a
>> allyesconfig on an architecture that does not provide HAS_IOMEM. To avoid
>> these kinds of build errors and tedious fixup patches the idea is to provide
>> a stub function when HAS_IOMEM is not enabled, but COMPILE_TEST is enabled.
>
> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
> to build on UML seems pointless to me and we special-case it in a number of
> places already.
>
According to current source code, tile still has chance to choose
NO_IOMEM, for me, welcome the tile's maintainer's ideas or suggestions.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-17 11:32 UTC (permalink / raw)
To: Richard Weinberger, Arnd Bergmann
Cc: Guenter Roeck, Lars-Peter Clausen, Greg Kroah-Hartman,
dmitry.torokhov, linux-iio, Benjamin Herrenschmidt, teg,
Thierry Reding, Lennox Wu, Marek Vasut, Liqin Chen, msalter,
linux-pwm, devel, linux-watchdog, linux-input,
linux-kernel@vger.kernel.org, knaack.h, Martin Schwidefsky,
Mischa.Jonker, jic23, Geert Uytterhoeven
In-Reply-To: <53C7AC6F.1070001@nod.at>
On 07/17/2014 06:58 PM, Richard Weinberger wrote:
> Am 17.07.2014 12:28, schrieb Arnd Bergmann:
>> On Thursday 17 July 2014 11:26:57 Richard Weinberger wrote:
>>> Am 17.07.2014 11:20, schrieb Arnd Bergmann:
>>>> On Thursday 17 July 2014 09:27:58 Chen Gang wrote:
>>>>> gfp_t gfp_mask, unsigned int order);
>>>>> extern void devm_free_pages(struct device *dev, unsigned long addr);
>>>>>
>>>>> +#ifdef CONFIG_HAS_IOMEM
>>>>> void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
>>>>> +#elif defined(CONFIG_COMPILE_TEST)
>>>>> +static inline void __iomem *devm_ioremap_resource(struct device *dev,
>>>>> + struct resource *res)
>>>>> +{
>>>>> + pr_warn("no hardware io memory, only for COMPILE_TEST\n");
>>>>> + return (__force void __iomem *)ERR_PTR(-ENXIO);
>>>>> +}
>>>>> +#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */
>>>>>
>>>>> /* allows to add/remove a custom action to devres stack */
>>>>
>>>> To be honest, I think it's a bad idea to introduce wrappers functions
>>>> that are only available when CONFIG_COMPILE_TEST is set.
>>>>
>>>> COMPILE_TEST is a great tool in general, but it has its limits.
>>>> In particular, the case for !CONFIG_IOMEM is completely obscure
>>>> and we won't find any bugs by allowing more drivers to be built
>>>> in those configurations, but attempting to do it would cause
>>>> endless churn by changing each instance of 'depends on HAS_IOMEM'
>>>> to 'depends on HAS_IOMEM || COMPILE_TEST'.
>>>>
>>>> Note that s390 no has gained support for IOMEM, tile has it most
>>>> of the time (when PCI is enabled, so you get it in half the
>>>> test builds already), score should set HAS_IOMEM and doesn't
>>>> even have public compilers, and uml doesn't even compile in
>>>> latest mainline. Nothing else ever sets NO_IOMEM.
>>>
>>> Huh? UML (v3.16-rc5-143-gb6603fe) builds fine here. What build issue are you facing?
>>
>> This is what I got upon trying earlier. I have not attempted to look into
>> why this is happening. Note this is on linux-next from yesterday,
>> not mainline as I incorrectly stated above.
>>
>> In file included from ../arch/um/include/asm/fixmap.h:58:0,
>> from ../arch/um/include/asm/pgtable.h:11,
>> from ../include/linux/mm.h:51,
>> from ../kernel/uid16.c:6:
>> ../include/asm-generic/fixmap.h: In function 'fix_to_virt':
>> ../include/asm-generic/fixmap.h:31:2: error: size of unnamed array is negative
>> BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
>
> So, this is next-20140716?
> I don't see the fixmap issue you're reporting, also not on the most recent next.
>
> All I see is the known ext4 issue with CONFIG_SMP=n:
>
> fs/ext4/super.c: In function ‘ext4_commit_super’:
> fs/ext4/super.c:4547:41: error: ‘struct percpu_counter’ has no member named ‘counters’
> if (EXT4_SB(sb)->s_freeclusters_counter.counters)
> ^
> fs/ext4/super.c:4551:39: error: ‘struct percpu_counter’ has no member named ‘counters’
> if (EXT4_SB(sb)->s_freeinodes_counter.counters)
>
Yeah, and I have notified to ext4 related maintainer, yesterday, and he
has already send fix patch for it, I guess it will be integrated into
main line soon.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
--
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] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-17 11:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-iio, Benjamin Herrenschmidt, teg, Thierry Reding, Lennox Wu,
Marek Vasut, Liqin Chen, Lars-Peter Clausen, Richard Weinberger,
Geert Uytterhoeven, msalter, Guenter Roeck, linux-pwm, devel,
linux-watchdog, linux-input, Greg Kroah-Hartman, dmitry.torokhov,
linux-kernel@vger.kernel.org, knaack.h, Martin Schwidefsky, jic23
In-Reply-To: <7018119.hVYUML2rzD@wuerfel>
On 07/17/2014 06:38 PM, Arnd Bergmann wrote:
> On Thursday 17 July 2014 17:29:31 Chen Gang wrote:
>>>
>>> COMPILE_TEST is a great tool in general, but it has its limits.
>>> In particular, the case for !CONFIG_IOMEM is completely obscure
>>> and we won't find any bugs by allowing more drivers to be built
>>> in those configurations, but attempting to do it would cause
>>> endless churn by changing each instance of 'depends on HAS_IOMEM'
>>> to 'depends on HAS_IOMEM || COMPILE_TEST'.
>>>
>>
>> Architecture members and driver members really have different tastes,
>> they are different roles. It really need additional discussion.
>>
>> For me, I only want to change devm_io*map*, not touch so much.
>
> But what do you gain from that? All drivers that need these
> functions should already 'depends on HAS_IOMEM' and if they don't,
> we should fix /that/ instead. I don't see this dependency as any
> different from a lot of others (PCI, DMAENGINE, HAVE_CLK, ...)
> that we use to intentionally annotate drivers that need a particular
> feature to be present for compilation. Do you want to do the
> same hack to those?
>
>> Welcome any other members' idea or suggestions.
>
>>> Note that s390 no has gained support for IOMEM, tile has it most
>>> of the time (when PCI is enabled, so you get it in half the
>>> test builds already), score should set HAS_IOMEM and doesn't
>>> even have public compilers, and uml doesn't even compile in
>>> latest mainline. Nothing else ever sets NO_IOMEM.
>>>
>>
I guess, we are just discussing about them in another threads, so I skip
them. If it is still necessary to reply (e.g. I misunderstand), please
let me know, thanks.
>> In latest gcc and binutils, can compile score cross compiler
>> successfully for building kernel (but I am not quite sure whether the
>> compiling result are really OK, but I guess so).
>
> Ok. Would you mind sending a patch that enables HAS_IOMEM on
> score?
>
For me, welcome the score related maintainers' idea and suggestions.
>> And next (maybe after finish allmodconfig for microblaze), I shall try
>> to let uml pass allmodconfig for linux-next tree.
>
> That is a fair goal, but it seems better to do that by ensuring
> we don't build any code that tries to call the MMIO functions
> rather than trying to make them build.
>
When I am performing uml, I will try and also communicate with the
related maintainers for it (their suggestions and ideas are valuable).
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Alan Stern @ 2014-07-17 14:39 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Bastien Nocera, Rafael J. Wysocki, Patrik Fimml, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <CAE_wzQ8HERY+EX_5kjfHML-eu2cxv5ihhYt2nCn34Zws4mgS-A@mail.gmail.com>
On Wed, 16 Jul 2014, Dmitry Torokhov wrote:
> We are not planning on implementing the policy in kernel, that's
> indeed task for userspace; but unless we bring in the heavy hammer of
> forcibly unbinding drivers, we do not currently have universal
> mechanism of quiescing devices.
We sort of do: the ->freeze() callback. But it wasn't intended for
this kind of use; drivers may very well expect that userspace will
already be frozen when the callback runs. Besides, ->freeze() is
supposed to quiesce devices without powering them down, whereas you
want to do both.
What you're asking for is different from anything the PM subsystem has
done before. Given this fact, I don't see any alternatives to adding a
new API or repurposing an existing API. Either one would be somewhat
painful.
For example, we could arrange to invoke ->suspend(). However, since
the circumstances would be unusual (userspace is still running,
->prepare() was not called beforehand, ->suspend_irq() won't be called
afterward), subsystems and drivers may very well react inappropriately.
Alan Stern
^ permalink raw reply
* Re: [PATCH] HID: rmi: check that report ids exist in the report_id_hash before accessing their size
From: Benjamin Tissoires @ 2014-07-17 16:07 UTC (permalink / raw)
To: Andrew Duggan; +Cc: linux-input, linux-kernel, Jiri Kosina
In-Reply-To: <1405546717-6582-1-git-send-email-aduggan@synaptics.com>
On Jul 16 2014 or thereabouts, Andrew Duggan wrote:
> It is possible that the hid-rmi driver could get loaded onto a device which does not have the
> expected report ids. This should not happen because it would indicate that the hid-rmi driver is
> not compatible with that device. However, if it does happen it should return an error from probe
> instead of dereferencing a null pointer.
>
> related bug:
> https://bugzilla.kernel.org/show_bug.cgi?id=80091
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> ---
Hi Andrew,
I think this patch is required. We can not afford having a null pointer
in a driver. So yes, I think Jiri should take this one. I just have a
few nitpick inlined in the code.
However, this patch will prevent the driver to fail, but the falsely
bound device will not be driven by anybody, which means that you will
break the keyboard/touchpad/whatever which is not using the right
driver.
So yes, we need that, but we also defintively have to fix hid-core too
to not bind them to hid-rmi.
> drivers/hid/hid-rmi.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 3221a95..7f0927a 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -848,6 +848,8 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
> struct rmi_data *data = NULL;
> int ret;
> size_t alloc_size;
> + struct hid_report *input_report;
> + struct hid_report *output_report;
>
> data = devm_kzalloc(&hdev->dev, sizeof(struct rmi_data), GFP_KERNEL);
> if (!data)
> @@ -866,12 +868,24 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> - data->input_report_size = (hdev->report_enum[HID_INPUT_REPORT]
> - .report_id_hash[RMI_ATTN_REPORT_ID]->size >> 3)
> - + 1 /* report id */;
> - data->output_report_size = (hdev->report_enum[HID_OUTPUT_REPORT]
> - .report_id_hash[RMI_WRITE_REPORT_ID]->size >> 3)
> - + 1 /* report id */;
> + input_report = hdev->report_enum[HID_INPUT_REPORT]
> + .report_id_hash[RMI_ATTN_REPORT_ID];
> + if (!input_report) {
> + ret = -ENODEV;
> + return ret;
In its current state, the driver will be silently unbound, and the user
will never get any explanations on why nothing works. So please add an
hid_err message telling why hid-rmi gave up.
> + }
> +
> + data->input_report_size = (input_report->size >> 3) + 1 /* report id */;
> +
> + output_report = hdev->report_enum[HID_OUTPUT_REPORT]
> + .report_id_hash[RMI_WRITE_REPORT_ID];
> + if (!output_report) {
> + ret = -ENODEV;
> + return ret;
ditto.
Cheers,
Benjamin
> + }
> +
> + data->output_report_size = (output_report->size >> 3)
> + + 1 /* report id */;
>
> alloc_size = data->output_report_size + data->input_report_size;
>
> --
> 1.9.1
>
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Dmitry Torokhov @ 2014-07-17 16:59 UTC (permalink / raw)
To: Alan Stern
Cc: Bastien Nocera, Rafael J. Wysocki, Patrik Fimml, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <Pine.LNX.4.44L0.1407171029210.1517-100000@iolanthe.rowland.org>
On Thursday, July 17, 2014 10:39:16 AM Alan Stern wrote:
> On Wed, 16 Jul 2014, Dmitry Torokhov wrote:
> > We are not planning on implementing the policy in kernel, that's
> > indeed task for userspace; but unless we bring in the heavy hammer of
> > forcibly unbinding drivers, we do not currently have universal
> > mechanism of quiescing devices.
>
> We sort of do: the ->freeze() callback. But it wasn't intended for
> this kind of use; drivers may very well expect that userspace will
> already be frozen when the callback runs. Besides, ->freeze() is
> supposed to quiesce devices without powering them down, whereas you
> want to do both.
Right.
>
> What you're asking for is different from anything the PM subsystem has
> done before.
Right.
> Given this fact, I don't see any alternatives to adding a
> new API or repurposing an existing API. Either one would be somewhat
> painful.
>
> For example, we could arrange to invoke ->suspend(). However, since
> the circumstances would be unusual (userspace is still running,
> ->prepare() was not called beforehand, ->suspend_irq() won't be called
> afterward), subsystems and drivers may very well react inappropriately.
I do not think anybody expects that drivers would not have to be modified to
support this functionality; I expect drivers would have to declare themselves
"queiscable" and therefore would assert that they will act according to
whatever rules we set up. I only want to make sure that this new state is
added to existing list of PM states rather than creating completely new
facility, so that driver authors have a chance to understand PM state
transitions that involve their driver.
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: call the hid driver's suspend and resume callbacks
From: Benjamin Tissoires @ 2014-07-17 17:00 UTC (permalink / raw)
To: Andrew Duggan
Cc: linux-input, linux-kernel, Jiri Kosina, Vincent Huang,
Mika Westerberg
In-Reply-To: <1405121658-3853-1-git-send-email-aduggan@synaptics.com>
On Jul 11 2014 or thereabouts, Andrew Duggan wrote:
> Currently, the i2c-hid driver does not call the suspend, resume, and
> reset_resume callbacks in the hid_driver struct when those events occur.
> This means that HID drivers for i2c-hid devices will not be able to execute
> commands which may be needed during suspend or resume. One example is when a
> touchpad using the hid-multitouch driver gets reset by i2c-hid coming out of
> resume. Since the reset_resume callback never gets called the device is never
> put back into the correct input mode. This patch calls the suspend and resume
> callbacks and tries to duplicate the functionality of the usb-hid driver.
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> Signed-off-by: Vincent Huang <vincent.huang@tw.synaptics.com>
> ---
> Hi Benjamin,
>
> This is the patch which Vincent and I came up with to fix the behavior which we were seeing with touchpads not being operational after resuming. We only use the reset_resume callback since i2c_hid_resume is always doing a hardware reset and it doesn't seem like there is a condition in which the standard resume callback would be appropriate. Also, is a full hardware reset always needed when resuming?
Hi Andrew,
well, the PM management of i2c_hid has been handled by Mika (in CC) and
I am unfortunately not able to have a strong opinion on this. I still
lack of hardware properly supporting the suspend/resume mechanism.
So, you are in charge of that. IIRC, nothing prevents from not calling
hw_reset, but I guess this was the safer implementation we could have.
Anyway, the patch looks good:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
>
> drivers/hid/i2c-hid/i2c-hid.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 21aafc8..747d544 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -1054,21 +1054,29 @@ static int i2c_hid_remove(struct i2c_client *client)
> static int i2c_hid_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> + struct i2c_hid *ihid = i2c_get_clientdata(client);
> + struct hid_device *hid = ihid->hid;
> + int ret = 0;
>
> disable_irq(client->irq);
> if (device_may_wakeup(&client->dev))
> enable_irq_wake(client->irq);
>
> + if (hid->driver && hid->driver->suspend)
> + ret = hid->driver->suspend(hid, PMSG_SUSPEND);
> +
> /* Save some power */
> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
>
> - return 0;
> + return ret;
> }
>
> static int i2c_hid_resume(struct device *dev)
> {
> int ret;
> struct i2c_client *client = to_i2c_client(dev);
> + struct i2c_hid *ihid = i2c_get_clientdata(client);
> + struct hid_device *hid = ihid->hid;
>
> enable_irq(client->irq);
> ret = i2c_hid_hwreset(client);
> @@ -1078,6 +1086,11 @@ static int i2c_hid_resume(struct device *dev)
> if (device_may_wakeup(&client->dev))
> disable_irq_wake(client->irq);
>
> + if (hid->driver && hid->driver->reset_resume) {
> + ret = hid->driver->reset_resume(hid);
> + return ret;
> + }
> +
> return 0;
> }
> #endif
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Richard Weinberger @ 2014-07-17 18:09 UTC (permalink / raw)
To: Arnd Bergmann, Lars-Peter Clausen
Cc: linux-pwm, linux-iio, Benjamin Herrenschmidt, teg, Thierry Reding,
Lennox Wu, linux-watchdog, Marek Vasut, Liqin Chen,
Geert Uytterhoeven, msalter, Guenter Roeck, devel, Chen Gang,
linux-input, Greg Kroah-Hartman, dmitry.torokhov,
linux-kernel@vger.kernel.org, knaack.h, Martin Schwidefsky,
Mischa.Jonker, jic23
In-Reply-To: <6485666.LVFO2YCEQx@wuerfel>
Am 17.07.2014 12:48, schrieb Arnd Bergmann:
> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
> to build on UML seems pointless to me and we special-case it in a number of
> places already.
If UML is the only arch without io memory the dependency on !UML seems
reasonable to me. :-)
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chris Metcalf @ 2014-07-17 20:41 UTC (permalink / raw)
To: Chen Gang, Arnd Bergmann, Lars-Peter Clausen
Cc: Guenter Roeck, Richard Weinberger, Greg Kroah-Hartman,
dmitry.torokhov, linux-iio, Benjamin Herrenschmidt, teg,
Thierry Reding, Lennox Wu, Marek Vasut, Liqin Chen, msalter,
linux-pwm, devel, linux-watchdog, linux-input,
linux-kernel@vger.kernel.org, knaack.h, Martin Schwidefsky,
Mischa.Jonker, jic23, Geert Uytterhoeven
In-Reply-To: <53C7B365.4080000@gmail.com>
On 7/17/2014 7:28 AM, Chen Gang wrote:
> On 07/17/2014 06:48 PM, Arnd Bergmann wrote:
>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>> to build on UML seems pointless to me and we special-case it in a number of
>> places already.
>>
> According to current source code, tile still has chance to choose
> NO_IOMEM, for me, welcome the tile's maintainer's ideas or suggestions.
I'm not really sure. It's true that on tile, if you don't enable PCI
support there's no other I/O memory (or I/O port) space you can use.
We pretty much always enable PCI support in our kernel, though. I'm
kind of surprised that other architectures don't also have the model
that IOMEM requires PCI, but perhaps most architectures just don't
encode that in the Kconfig file?
My observation is just that if I remove the "NO_IOMEM if !PCI" from
arch/tile/Kconfig, my build fails with ioremap() undefined. No doubt I
could work around that, but my assumption was that NO_IOMEM was exactly the
right thing to express the fact that without PCI there is no I/O memory :-)
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Arnd Bergmann @ 2014-07-17 21:05 UTC (permalink / raw)
To: Chris Metcalf
Cc: linux-iio, Benjamin Herrenschmidt, teg, Thierry Reding, Lennox Wu,
linux-watchdog, Marek Vasut, Liqin Chen, Lars-Peter Clausen,
Richard Weinberger, Geert Uytterhoeven, msalter, Guenter Roeck,
linux-pwm, devel, Chen Gang, linux-input, Greg Kroah-Hartman,
dmitry.torokhov, linux-kernel@vger.kernel.org, knaack.h,
Martin Schwidefsky, Mischa.Jonker, jic23
In-Reply-To: <53C834EA.5030605@tilera.com>
On Thursday 17 July 2014 16:41:14 Chris Metcalf wrote:
> On 7/17/2014 7:28 AM, Chen Gang wrote:
> > On 07/17/2014 06:48 PM, Arnd Bergmann wrote:
> >> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
> >> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
> >> to build on UML seems pointless to me and we special-case it in a number of
> >> places already.
> >>
> > According to current source code, tile still has chance to choose
> > NO_IOMEM, for me, welcome the tile's maintainer's ideas or suggestions.
>
> I'm not really sure. It's true that on tile, if you don't enable PCI
> support there's no other I/O memory (or I/O port) space you can use.
> We pretty much always enable PCI support in our kernel, though. I'm
> kind of surprised that other architectures don't also have the model
> that IOMEM requires PCI, but perhaps most architectures just don't
> encode that in the Kconfig file?
Only s390 as far as I know. Most architectures have integrated
peripherals that use MMIO without PCI.
> My observation is just that if I remove the "NO_IOMEM if !PCI" from
> arch/tile/Kconfig, my build fails with ioremap() undefined. No doubt I
> could work around that, but my assumption was that NO_IOMEM was exactly the
> right thing to express the fact that without PCI there is no I/O memory
Your assumption is correct.
For tile by itself it would certainly be best to leave this
dependency, it makes no sense to enable IOMEM without PCI.
That doesn't solve the problem of COMPILE_TEST enabling drivers
that require IOMEM though. An easy hack for that would be to
make COMPILE_TEST depend on HAS_IOMEM, but it gets into hacky territory
there, and it's not clear if this is any better than the original patch
to provide fallbacks for ioremap and friends. Definitely simpler
though.
Arnd
^ permalink raw reply
* [PATCH] HID: rmi: only bind the hid-rmi driver to the mouse interface of composite USB devices
From: Andrew Duggan @ 2014-07-17 23:14 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Andrew Duggan, Jiri Kosina, Benjamin Tissoires
In-Reply-To: <1405638885-8014-1-git-send-email-aduggan@synaptics.com>
On composite HID devices there may be multiple HID devices on separate interfaces, but hid-rmi
should only bind to the mouse interface. One example is the Dell Venue 11 Pro's keyboard dock
which contains a composite USB device with a HID touchpad and HID keyboard on separate intefaces.
Since the USB Vendor ID is Synaptic's, hid-core is currently trying to bind hid-rmi to all\of
the HID devices. This patch ensures that hid-rmi only binds to the mouse interface.
related bug:
https://bugzilla.kernel.org/show_bug.cgi?id=80091
Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
drivers/hid/hid-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8ed66fd..f10e768 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -783,7 +783,9 @@ static int hid_scan_report(struct hid_device *hid)
* Vendor specific handlings
*/
if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
- (hid->group == HID_GROUP_GENERIC))
+ (hid->group == HID_GROUP_GENERIC) &&
+ /* only bind to the mouse interface of composite USB devices */
+ (hid->bus != BUS_USB || hid->type == HID_TYPE_USBMOUSE))
/* hid-rmi should take care of them, not hid-generic */
hid->group = HID_GROUP_RMI;
--
1.9.1
^ permalink raw reply related
* [PATCH v2] HID: rmi: check that report ids exist in the report_id_hash before accessing their size
From: Andrew Duggan @ 2014-07-17 23:14 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Andrew Duggan, Jiri Kosina, Benjamin Tissoires
It is possible that the hid-rmi driver could get loaded onto a device which does not have the
expected report ids. This should not happen because it would indicate that the hid-rmi driver is
not compatible with that device. However, if it does happen it should return an error from probe
instead of dereferencing a null pointer.
related bug:
https://bugzilla.kernel.org/show_bug.cgi?id=80091
Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
---
Added hid_err messages as suggested by Benjamin. This fixes the null pointer dereference from
bug 80091 while the next patch addresses the binding issue in hid-core.
drivers/hid/hid-rmi.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 3221a95..260be7a 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -848,6 +848,8 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
struct rmi_data *data = NULL;
int ret;
size_t alloc_size;
+ struct hid_report *input_report;
+ struct hid_report *output_report;
data = devm_kzalloc(&hdev->dev, sizeof(struct rmi_data), GFP_KERNEL);
if (!data)
@@ -866,12 +868,26 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- data->input_report_size = (hdev->report_enum[HID_INPUT_REPORT]
- .report_id_hash[RMI_ATTN_REPORT_ID]->size >> 3)
- + 1 /* report id */;
- data->output_report_size = (hdev->report_enum[HID_OUTPUT_REPORT]
- .report_id_hash[RMI_WRITE_REPORT_ID]->size >> 3)
- + 1 /* report id */;
+ input_report = hdev->report_enum[HID_INPUT_REPORT]
+ .report_id_hash[RMI_ATTN_REPORT_ID];
+ if (!input_report) {
+ hid_err(hdev, "device does not have expected input report\n");
+ ret = -ENODEV;
+ return ret;
+ }
+
+ data->input_report_size = (input_report->size >> 3) + 1 /* report id */;
+
+ output_report = hdev->report_enum[HID_OUTPUT_REPORT]
+ .report_id_hash[RMI_WRITE_REPORT_ID];
+ if (!output_report) {
+ hid_err(hdev, "device does not have expected output report\n");
+ ret = -ENODEV;
+ return ret;
+ }
+
+ data->output_report_size = (output_report->size >> 3)
+ + 1 /* report id */;
alloc_size = data->output_report_size + data->input_report_size;
--
1.9.1
^ permalink raw reply related
* Re: Power-managing devices that are not of interest at some point in time
From: Rafael J. Wysocki @ 2014-07-18 0:43 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alan Stern, Bastien Nocera, Patrik Fimml, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <3088260.XkiUM6Vtxh@dtor-glaptop>
On Thursday, July 17, 2014 09:59:19 AM Dmitry Torokhov wrote:
> On Thursday, July 17, 2014 10:39:16 AM Alan Stern wrote:
> > On Wed, 16 Jul 2014, Dmitry Torokhov wrote:
> > > We are not planning on implementing the policy in kernel, that's
> > > indeed task for userspace; but unless we bring in the heavy hammer of
> > > forcibly unbinding drivers, we do not currently have universal
> > > mechanism of quiescing devices.
> >
> > We sort of do: the ->freeze() callback. But it wasn't intended for
> > this kind of use; drivers may very well expect that userspace will
> > already be frozen when the callback runs. Besides, ->freeze() is
> > supposed to quiesce devices without powering them down, whereas you
> > want to do both.
>
> Right.
>
> >
> > What you're asking for is different from anything the PM subsystem has
> > done before.
>
> Right.
>
> > Given this fact, I don't see any alternatives to adding a
> > new API or repurposing an existing API. Either one would be somewhat
> > painful.
> >
> > For example, we could arrange to invoke ->suspend(). However, since
> > the circumstances would be unusual (userspace is still running,
> > ->prepare() was not called beforehand, ->suspend_irq() won't be called
> > afterward), subsystems and drivers may very well react inappropriately.
>
> I do not think anybody expects that drivers would not have to be modified to
> support this functionality; I expect drivers would have to declare themselves
> "queiscable" and therefore would assert that they will act according to
> whatever rules we set up. I only want to make sure that this new state is
> added to existing list of PM states rather than creating completely new
> facility, so that driver authors have a chance to understand PM state
> transitions that involve their driver.
If you're referring to runtime PM, it doesn't use "states". It uses status
values (you can think of them as metastates) which are "active", "suspended"
or in-transit from one to the other. There's no room for more of these in
the design, I'm afraid.
Moreover, .runtime_suspend() can only be called when the device is quiescent
already. [That also applies to .suspend_late() and .suspend_irq() for
system suspend and the freezing of tasks is requisite for the .prepare() and
.suspend() callbacks (and the corresponding hibernation-related ones).]
>From past discussions on similar topics it followed that there really was
no generic way for individual drivers to quiesce devices on demand as long as
user space was running. Everything we could come up with was racy, this way or
another. That is the reason for using the freezer during system suspend. In
other words, if you want drivers to quiesce devices by force, you need to quiesce
user space by force to start with - for example by freezing it.
For runtime PM, on the other hand, the underlying observation is that drivers
should be able to detect when devices are already quiescent and initialize power
transitions at those points. It's role is to help with that, but not with quiescing
things.
That said, in the "laptop lid closed" scenario (assuming that the system is not
supposed to suspend in response to that, which in my opinion is the best approach)
the problem really seems to be that drivers are not aggressive enough with starting
PM transitions (using runtime PM) when they see no activity. Thus it seems that
when the lid is closed, it'll be good to switch the drivers into a "more aggressive
runtime PM mode" in which they will use any opportunity to start a power transition
without worrying about extra latencies resulting from that. In that mode they
should also disable remote wakeup. I think this should be sufficient to address
the use case at hand.
Rafael
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-18 0:26 UTC (permalink / raw)
To: Arnd Bergmann, Chris Metcalf
Cc: Lars-Peter Clausen, Guenter Roeck, Richard Weinberger,
Greg Kroah-Hartman, dmitry.torokhov, linux-iio,
Benjamin Herrenschmidt, teg, Thierry Reding, Lennox Wu,
Marek Vasut, Liqin Chen, msalter, linux-pwm, devel,
linux-watchdog, linux-input, linux-kernel@vger.kernel.org,
knaack.h, Martin Schwidefsky, Mischa.Jonker, jic23,
Geert Uytterhoeven
In-Reply-To: <5190668.jExaFa8pKn@wuerfel>
On 07/18/2014 05:05 AM, Arnd Bergmann wrote:
> On Thursday 17 July 2014 16:41:14 Chris Metcalf wrote:
>> On 7/17/2014 7:28 AM, Chen Gang wrote:
>>> On 07/17/2014 06:48 PM, Arnd Bergmann wrote:
>>>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>>>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>>>> to build on UML seems pointless to me and we special-case it in a number of
>>>> places already.
>>>>
>>> According to current source code, tile still has chance to choose
>>> NO_IOMEM, for me, welcome the tile's maintainer's ideas or suggestions.
>>
>> I'm not really sure. It's true that on tile, if you don't enable PCI
>> support there's no other I/O memory (or I/O port) space you can use.
>> We pretty much always enable PCI support in our kernel, though. I'm
>> kind of surprised that other architectures don't also have the model
>> that IOMEM requires PCI, but perhaps most architectures just don't
>> encode that in the Kconfig file?
>
> Only s390 as far as I know. Most architectures have integrated
> peripherals that use MMIO without PCI.
>
>> My observation is just that if I remove the "NO_IOMEM if !PCI" from
>> arch/tile/Kconfig, my build fails with ioremap() undefined. No doubt I
>> could work around that, but my assumption was that NO_IOMEM was exactly the
>> right thing to express the fact that without PCI there is no I/O memory
>
> Your assumption is correct.
>
> For tile by itself it would certainly be best to leave this
> dependency, it makes no sense to enable IOMEM without PCI.
>
> That doesn't solve the problem of COMPILE_TEST enabling drivers
> that require IOMEM though. An easy hack for that would be to
> make COMPILE_TEST depend on HAS_IOMEM, but it gets into hacky territory
> there, and it's not clear if this is any better than the original patch
> to provide fallbacks for ioremap and friends. Definitely simpler
> though.
>
OK, thank all of you, tile just likes most of architectures to support
IOMEM, and at present, we can focus score and uml only.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-18 0:36 UTC (permalink / raw)
To: Richard Weinberger, Arnd Bergmann, Lars-Peter Clausen
Cc: Guenter Roeck, Greg Kroah-Hartman, dmitry.torokhov, linux-iio,
Benjamin Herrenschmidt, teg, Thierry Reding, Lennox Wu,
Marek Vasut, Liqin Chen, msalter, linux-pwm, devel,
linux-watchdog, linux-input, linux-kernel@vger.kernel.org,
knaack.h, Martin Schwidefsky, Mischa.Jonker, jic23,
Geert Uytterhoeven
In-Reply-To: <53C8114D.30601@nod.at>
On 07/18/2014 02:09 AM, Richard Weinberger wrote:
> Am 17.07.2014 12:48, schrieb Arnd Bergmann:
>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>> to build on UML seems pointless to me and we special-case it in a number of
>> places already.
>
> If UML is the only arch without io memory the dependency on !UML seems
> reasonable to me. :-)
>
For me, if only uml left, I suggest to implement dummy functions within
uml instead of let CONFIG_UML appear in generic include directory. And
then remove all HAS_IOMEM and NO_IOMEM from kernel.
If score sticks to NO_IOMEM, and can not remove score from kernel only
because of this reason. I also suggest implement dummy functions within
score itself.
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Dmitry Torokhov @ 2014-07-18 0:43 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Alan Stern, Bastien Nocera, Patrik Fimml, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <8587268.sIIBz6Y2QN@vostro.rjw.lan>
On Friday, July 18, 2014 02:43:02 AM Rafael J. Wysocki wrote:
> On Thursday, July 17, 2014 09:59:19 AM Dmitry Torokhov wrote:
> > On Thursday, July 17, 2014 10:39:16 AM Alan Stern wrote:
> > > On Wed, 16 Jul 2014, Dmitry Torokhov wrote:
> > > > We are not planning on implementing the policy in kernel, that's
> > > > indeed task for userspace; but unless we bring in the heavy hammer of
> > > > forcibly unbinding drivers, we do not currently have universal
> > > > mechanism of quiescing devices.
> > >
> > > We sort of do: the ->freeze() callback. But it wasn't intended for
> > > this kind of use; drivers may very well expect that userspace will
> > > already be frozen when the callback runs. Besides, ->freeze() is
> > > supposed to quiesce devices without powering them down, whereas you
> > > want to do both.
> >
> > Right.
> >
> > > What you're asking for is different from anything the PM subsystem has
> > > done before.
> >
> > Right.
> >
> > > Given this fact, I don't see any alternatives to adding a
> > > new API or repurposing an existing API. Either one would be somewhat
> > > painful.
> > >
> > > For example, we could arrange to invoke ->suspend(). However, since
> > > the circumstances would be unusual (userspace is still running,
> > > ->prepare() was not called beforehand, ->suspend_irq() won't be called
> > > afterward), subsystems and drivers may very well react inappropriately.
> >
> > I do not think anybody expects that drivers would not have to be modified
> > to support this functionality; I expect drivers would have to declare
> > themselves "queiscable" and therefore would assert that they will act
> > according to whatever rules we set up. I only want to make sure that this
> > new state is added to existing list of PM states rather than creating
> > completely new facility, so that driver authors have a chance to
> > understand PM state transitions that involve their driver.
>
> If you're referring to runtime PM, it doesn't use "states". It uses status
> values (you can think of them as metastates) which are "active", "suspended"
> or in-transit from one to the other. There's no room for more of these in
> the design, I'm afraid.
>
> Moreover, .runtime_suspend() can only be called when the device is quiescent
> already. [That also applies to .suspend_late() and .suspend_irq() for
> system suspend and the freezing of tasks is requisite for the .prepare()
> and .suspend() callbacks (and the corresponding hibernation-related ones).]
>
> From past discussions on similar topics it followed that there really was
> no generic way for individual drivers to quiesce devices on demand as long
> as user space was running. Everything we could come up with was racy, this
> way or another. That is the reason for using the freezer during system
> suspend. In other words, if you want drivers to quiesce devices by force,
> you need to quiesce user space by force to start with - for example by
> freezing it.
>
> For runtime PM, on the other hand, the underlying observation is that
> drivers should be able to detect when devices are already quiescent and
> initialize power transitions at those points. It's role is to help with
> that, but not with quiescing things.
>
> That said, in the "laptop lid closed" scenario (assuming that the system is
> not supposed to suspend in response to that, which in my opinion is the
> best approach)
This is default approach that works for many, but not necessarily all use
cases. I believe docked with lid closed scenario was mentioned already.
> the problem really seems to be that drivers are not
> aggressive enough with starting PM transitions (using runtime PM) when they
> see no activity. Thus it seems that when the lid is closed, it'll be good
> to switch the drivers into a "more aggressive runtime PM mode" in which
> they will use any opportunity to start a power transition without worrying
> about extra latencies resulting from that. In that mode they should also
> disable remote wakeup. I think this should be sufficient to address the
> use case at hand.
OK, so how do we let the drivers know that they should start being aggressive
with PM and that they should disable remote wakeup? I'd rather not add
subsystem-specific interface for this, that is why we are reaching out in the
first place.
Thanks,
Dmitry
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Rafael J. Wysocki @ 2014-07-18 1:30 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alan Stern, Bastien Nocera, Patrik Fimml, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <4468130.Uu9vndH3sC@dtor-glaptop>
On Thursday, July 17, 2014 05:43:42 PM Dmitry Torokhov wrote:
> On Friday, July 18, 2014 02:43:02 AM Rafael J. Wysocki wrote:
> > On Thursday, July 17, 2014 09:59:19 AM Dmitry Torokhov wrote:
> > > On Thursday, July 17, 2014 10:39:16 AM Alan Stern wrote:
> > > > On Wed, 16 Jul 2014, Dmitry Torokhov wrote:
> > > > > We are not planning on implementing the policy in kernel, that's
> > > > > indeed task for userspace; but unless we bring in the heavy hammer of
> > > > > forcibly unbinding drivers, we do not currently have universal
> > > > > mechanism of quiescing devices.
> > > >
> > > > We sort of do: the ->freeze() callback. But it wasn't intended for
> > > > this kind of use; drivers may very well expect that userspace will
> > > > already be frozen when the callback runs. Besides, ->freeze() is
> > > > supposed to quiesce devices without powering them down, whereas you
> > > > want to do both.
> > >
> > > Right.
> > >
> > > > What you're asking for is different from anything the PM subsystem has
> > > > done before.
> > >
> > > Right.
> > >
> > > > Given this fact, I don't see any alternatives to adding a
> > > > new API or repurposing an existing API. Either one would be somewhat
> > > > painful.
> > > >
> > > > For example, we could arrange to invoke ->suspend(). However, since
> > > > the circumstances would be unusual (userspace is still running,
> > > > ->prepare() was not called beforehand, ->suspend_irq() won't be called
> > > > afterward), subsystems and drivers may very well react inappropriately.
> > >
> > > I do not think anybody expects that drivers would not have to be modified
> > > to support this functionality; I expect drivers would have to declare
> > > themselves "queiscable" and therefore would assert that they will act
> > > according to whatever rules we set up. I only want to make sure that this
> > > new state is added to existing list of PM states rather than creating
> > > completely new facility, so that driver authors have a chance to
> > > understand PM state transitions that involve their driver.
> >
> > If you're referring to runtime PM, it doesn't use "states". It uses status
> > values (you can think of them as metastates) which are "active", "suspended"
> > or in-transit from one to the other. There's no room for more of these in
> > the design, I'm afraid.
> >
> > Moreover, .runtime_suspend() can only be called when the device is quiescent
> > already. [That also applies to .suspend_late() and .suspend_irq() for
> > system suspend and the freezing of tasks is requisite for the .prepare()
> > and .suspend() callbacks (and the corresponding hibernation-related ones).]
> >
> > From past discussions on similar topics it followed that there really was
> > no generic way for individual drivers to quiesce devices on demand as long
> > as user space was running. Everything we could come up with was racy, this
> > way or another. That is the reason for using the freezer during system
> > suspend. In other words, if you want drivers to quiesce devices by force,
> > you need to quiesce user space by force to start with - for example by
> > freezing it.
> >
> > For runtime PM, on the other hand, the underlying observation is that
> > drivers should be able to detect when devices are already quiescent and
> > initialize power transitions at those points. It's role is to help with
> > that, but not with quiescing things.
> >
> > That said, in the "laptop lid closed" scenario (assuming that the system is
> > not supposed to suspend in response to that, which in my opinion is the
> > best approach)
>
> This is default approach that works for many, but not necessarily all use
> cases. I believe docked with lid closed scenario was mentioned already.
>
> > the problem really seems to be that drivers are not
> > aggressive enough with starting PM transitions (using runtime PM) when they
> > see no activity. Thus it seems that when the lid is closed, it'll be good
> > to switch the drivers into a "more aggressive runtime PM mode" in which
> > they will use any opportunity to start a power transition without worrying
> > about extra latencies resulting from that. In that mode they should also
> > disable remote wakeup. I think this should be sufficient to address the
> > use case at hand.
>
> OK, so how do we let the drivers know that they should start being aggressive
> with PM and that they should disable remote wakeup? I'd rather not add
> subsystem-specific interface for this, that is why we are reaching out in the
> first place.
For disabling remote wakeup we have a PM QoS flag that I'm not entirely happy
with, so I guess we can replace it with something saner (I talked about that
with Alan during the last year's LinuxCon, but then didn't have the time to
get to that).
For the whole thing I guess we can add a sysfs attribute under devices/.../power
that will need to be exposed by drivers supporting that feature. I'm not sure
how to call it, though.
Rafael
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Rafael J. Wysocki @ 2014-07-18 1:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Alan Stern, Bastien Nocera, Patrik Fimml, linux-pm, Benson Leung,
linux-input, linux-kernel
In-Reply-To: <2674541.jV2qHCZlHf@vostro.rjw.lan>
On Friday, July 18, 2014 03:30:31 AM Rafael J. Wysocki wrote:
> On Thursday, July 17, 2014 05:43:42 PM Dmitry Torokhov wrote:
> > On Friday, July 18, 2014 02:43:02 AM Rafael J. Wysocki wrote:
> > > On Thursday, July 17, 2014 09:59:19 AM Dmitry Torokhov wrote:
> > > > On Thursday, July 17, 2014 10:39:16 AM Alan Stern wrote:
> > > > > On Wed, 16 Jul 2014, Dmitry Torokhov wrote:
> > > > > > We are not planning on implementing the policy in kernel, that's
> > > > > > indeed task for userspace; but unless we bring in the heavy hammer of
> > > > > > forcibly unbinding drivers, we do not currently have universal
> > > > > > mechanism of quiescing devices.
> > > > >
> > > > > We sort of do: the ->freeze() callback. But it wasn't intended for
> > > > > this kind of use; drivers may very well expect that userspace will
> > > > > already be frozen when the callback runs. Besides, ->freeze() is
> > > > > supposed to quiesce devices without powering them down, whereas you
> > > > > want to do both.
> > > >
> > > > Right.
> > > >
> > > > > What you're asking for is different from anything the PM subsystem has
> > > > > done before.
> > > >
> > > > Right.
> > > >
> > > > > Given this fact, I don't see any alternatives to adding a
> > > > > new API or repurposing an existing API. Either one would be somewhat
> > > > > painful.
> > > > >
> > > > > For example, we could arrange to invoke ->suspend(). However, since
> > > > > the circumstances would be unusual (userspace is still running,
> > > > > ->prepare() was not called beforehand, ->suspend_irq() won't be called
> > > > > afterward), subsystems and drivers may very well react inappropriately.
> > > >
> > > > I do not think anybody expects that drivers would not have to be modified
> > > > to support this functionality; I expect drivers would have to declare
> > > > themselves "queiscable" and therefore would assert that they will act
> > > > according to whatever rules we set up. I only want to make sure that this
> > > > new state is added to existing list of PM states rather than creating
> > > > completely new facility, so that driver authors have a chance to
> > > > understand PM state transitions that involve their driver.
> > >
> > > If you're referring to runtime PM, it doesn't use "states". It uses status
> > > values (you can think of them as metastates) which are "active", "suspended"
> > > or in-transit from one to the other. There's no room for more of these in
> > > the design, I'm afraid.
> > >
> > > Moreover, .runtime_suspend() can only be called when the device is quiescent
> > > already. [That also applies to .suspend_late() and .suspend_irq() for
> > > system suspend and the freezing of tasks is requisite for the .prepare()
> > > and .suspend() callbacks (and the corresponding hibernation-related ones).]
> > >
> > > From past discussions on similar topics it followed that there really was
> > > no generic way for individual drivers to quiesce devices on demand as long
> > > as user space was running. Everything we could come up with was racy, this
> > > way or another. That is the reason for using the freezer during system
> > > suspend. In other words, if you want drivers to quiesce devices by force,
> > > you need to quiesce user space by force to start with - for example by
> > > freezing it.
> > >
> > > For runtime PM, on the other hand, the underlying observation is that
> > > drivers should be able to detect when devices are already quiescent and
> > > initialize power transitions at those points. It's role is to help with
> > > that, but not with quiescing things.
> > >
> > > That said, in the "laptop lid closed" scenario (assuming that the system is
> > > not supposed to suspend in response to that, which in my opinion is the
> > > best approach)
> >
> > This is default approach that works for many, but not necessarily all use
> > cases. I believe docked with lid closed scenario was mentioned already.
> >
> > > the problem really seems to be that drivers are not
> > > aggressive enough with starting PM transitions (using runtime PM) when they
> > > see no activity. Thus it seems that when the lid is closed, it'll be good
> > > to switch the drivers into a "more aggressive runtime PM mode" in which
> > > they will use any opportunity to start a power transition without worrying
> > > about extra latencies resulting from that. In that mode they should also
> > > disable remote wakeup. I think this should be sufficient to address the
> > > use case at hand.
> >
> > OK, so how do we let the drivers know that they should start being aggressive
> > with PM and that they should disable remote wakeup? I'd rather not add
> > subsystem-specific interface for this, that is why we are reaching out in the
> > first place.
>
> For disabling remote wakeup we have a PM QoS flag that I'm not entirely happy
> with, so I guess we can replace it with something saner (I talked about that
> with Alan during the last year's LinuxCon, but then didn't have the time to
> get to that).
>
> For the whole thing I guess we can add a sysfs attribute under devices/.../power
> that will need to be exposed by drivers supporting that feature. I'm not sure
> how to call it, though.
Or we could add an "aggressive" value to the devices/.../power/control attribute,
but then it will be difficult for user space to verify whether or not it is
supported for the given device.
Rafael
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Richard Weinberger @ 2014-07-18 7:35 UTC (permalink / raw)
To: Chen Gang, Arnd Bergmann, Lars-Peter Clausen
Cc: Marek Vasut, Liqin Chen, devel, msalter, linux-watchdog,
linux-pwm, linux-iio, Greg Kroah-Hartman, dmitry.torokhov,
linux-kernel@vger.kernel.org, jic23, teg, Thierry Reding,
Geert Uytterhoeven, Lennox Wu, Benjamin Herrenschmidt,
Martin Schwidefsky, linux-input, Mischa.Jonker, Guenter Roeck,
knaack.h
In-Reply-To: <53C86C10.2000608@gmail.com>
Am 18.07.2014 02:36, schrieb Chen Gang:
>
> On 07/18/2014 02:09 AM, Richard Weinberger wrote:
>> Am 17.07.2014 12:48, schrieb Arnd Bergmann:
>>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>>> to build on UML seems pointless to me and we special-case it in a number of
>>> places already.
>>
>> If UML is the only arch without io memory the dependency on !UML seems
>> reasonable to me. :-)
>>
>
> For me, if only uml left, I suggest to implement dummy functions within
> uml instead of let CONFIG_UML appear in generic include directory. And
> then remove all HAS_IOMEM and NO_IOMEM from kernel.
Erm, this is something completely different.
I thought we're focusing on COMPILE_TEST?
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH v4 1/4] Rename hid-lenovo-tpkbd to hid-lenovo
From: Jamie Lentin @ 2014-07-18 9:35 UTC (permalink / raw)
To: Antonio Ospite; +Cc: Jiri Kosina, Hans de Goede, linux-input, linux-kernel
In-Reply-To: <20140714114227.161b4bf0767e1bbf3268852c@ao2.it>
On Mon, 14 Jul 2014, Antonio Ospite wrote:
> On Sun, 13 Jul 2014 08:24:19 +0100
> Jamie Lentin <jm@lentin.co.uk> wrote:
>
>> Rename module and all functions within so we can add support for other
>> keyboards in the same file. Rename the _tp postfix to _tpkbd, to
>> signify functions relevant to the TP USB keyboard.
>>
>> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
>
> Reviewed-by: Antonio Ospite <ao2@ao2.it>
>
> Patch looks good to me, easier to validate now :)
>
> A few ideas below for some possible _future_ cleanups, but no reason to
> hold this one any longer IMHO.
>
> Ciao,
> Antonio
>
>> ---
>> ...er-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} | 0
>> drivers/hid/Kconfig | 14 +-
>> drivers/hid/Makefile | 2 +-
>> drivers/hid/hid-core.c | 2 +-
>> drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} | 185 +++++++++++----------
>> 5 files changed, 102 insertions(+), 101 deletions(-)
>> rename Documentation/ABI/testing/{sysfs-driver-hid-lenovo-tpkbd => sysfs-driver-hid-lenovo} (100%)
>> rename drivers/hid/{hid-lenovo-tpkbd.c => hid-lenovo.c} (64%)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd b/Documentation/ABI/testing/sysfs-driver-hid-lenovo
>> similarity index 100%
>> rename from Documentation/ABI/testing/sysfs-driver-hid-lenovo-tpkbd
>> rename to Documentation/ABI/testing/sysfs-driver-hid-lenovo
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 800c8b6..9b7acfc 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -331,18 +331,18 @@ config HID_LCPOWER
>> ---help---
>> Support for LC-Power RC1000MCE RF remote control.
>>
>> -config HID_LENOVO_TPKBD
>> - tristate "Lenovo ThinkPad USB Keyboard with TrackPoint"
>> +config HID_LENOVO
>> + tristate "Lenovo / Thinkpad devices"
>> depends on HID
>> select NEW_LEDS
>> select LEDS_CLASS
>> ---help---
>> - Support for the Lenovo ThinkPad USB Keyboard with TrackPoint.
>> + Support for Lenovo devices that are not fully compliant with HID standard.
>>
>> - Say Y here if you have a Lenovo ThinkPad USB Keyboard with TrackPoint
>> - and would like to use device-specific features like changing the
>> - sensitivity of the trackpoint, using the microphone mute button or
>> - controlling the mute and microphone mute LEDs.
>> + Say Y if you want support for the non-compliant features of the Lenovo
>> + Thinkpad standalone keyboards, e.g:
>
> This could become:
>
> Say Y if you want support for the non-HID-compliant features of Lenovo
> devices, e.g:
> ...
>
> because, in principle, the driver is not only about keyboards anymore.
True, although I'd be very surprised if anything else came up :)
>>
>> + - ThinkPad USB Keyboard with TrackPoint (supports extra LEDs and trackpoint
>> + configuration)
>>
>> config HID_LOGITECH
>> tristate "Logitech devices" if EXPERT
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index a6fa6ba..5e96be3 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -59,7 +59,7 @@ obj-$(CONFIG_HID_KENSINGTON) += hid-kensington.o
>> obj-$(CONFIG_HID_KEYTOUCH) += hid-keytouch.o
>> obj-$(CONFIG_HID_KYE) += hid-kye.o
>> obj-$(CONFIG_HID_LCPOWER) += hid-lcpower.o
>> -obj-$(CONFIG_HID_LENOVO_TPKBD) += hid-lenovo-tpkbd.o
>> +obj-$(CONFIG_HID_LENOVO) += hid-lenovo.o
>> obj-$(CONFIG_HID_LOGITECH) += hid-logitech.o
>> obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-dj.o
>> obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8ed66fd..55841bd 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1796,7 +1796,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_EASYPEN_M610X) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LABTEC, USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_LCPOWER, USB_DEVICE_ID_LCPOWER_LC1000 ) },
>> -#if IS_ENABLED(CONFIG_HID_LENOVO_TPKBD)
>> +#if IS_ENABLED(CONFIG_HID_LENOVO)
>> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>> #endif
>> { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
>> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo.c
>> similarity index 64%
>> rename from drivers/hid/hid-lenovo-tpkbd.c
>> rename to drivers/hid/hid-lenovo.c
>> index 2d25b6c..0320b96 100644
>> --- a/drivers/hid/hid-lenovo-tpkbd.c
>> +++ b/drivers/hid/hid-lenovo.c
>> @@ -20,8 +20,7 @@
>>
>> #include "hid-ids.h"
>>
>> -/* This is only used for the trackpoint part of the driver, hence _tp */
>> -struct tpkbd_data_pointer {
>> +struct lenovo_drvdata_tpkbd {
>> int led_state;
>> struct led_classdev led_mute;
>> struct led_classdev led_micmute;
>> @@ -35,7 +34,7 @@ struct tpkbd_data_pointer {
>>
>> #define map_key_clear(c) hid_map_usage_clear(hi, usage, bit, max, EV_KEY, (c))
>>
>> -static int tpkbd_input_mapping(struct hid_device *hdev,
>> +static int lenovo_input_mapping_tpkbd(struct hid_device *hdev,
>> struct hid_input *hi, struct hid_field *field,
>> struct hid_usage *usage, unsigned long **bit, int *max)
>> {
>> @@ -50,10 +49,10 @@ static int tpkbd_input_mapping(struct hid_device *hdev,
>>
>> #undef map_key_clear
>>
>> -static int tpkbd_features_set(struct hid_device *hdev)
>> +static int lenovo_features_set_tpkbd(struct hid_device *hdev)
>> {
>> struct hid_report *report;
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>
>
> I think the word "pointer" here is misleading, sometimes in other parts
> of the driver it is used as a synonym of trackpoint, but
> lenovo_drvdata_tpkbd also contains data about leds, so here it looks
> like it refers to C pointers. I'd call the variable tpkbd_data or
> tpkbd_drvdata, throughout the driver. What do you think?
Yes, the rationale behind the name really isn't obvious but all meanings
are sorta correct, so it's not that bad :P
I like tpkbd_drvdata better though, and can correct it in the compact
keyboard code too. I wanted to make the variable names match, but didn't
get around to it, so will do it next time around.
> And the driver could standardize on the word "trackpoint" where
> appropriate in order to eliminate any ambiguity.
The key other place I can think of that this happens is for the USB
compact keyboard, for it's HID_TYPE_USBMOUSE half (versus the keyboard
half). Saying trackpoint there is a mis-truth, since it's also where all
the extra keys hide. But I'll have a look through anyway.
> I know, these are not your faults :)
>
>>
>> report = hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[4];
>>
>> @@ -69,23 +68,23 @@ static int tpkbd_features_set(struct hid_device *hdev)
>> return 0;
>> }
>>
>> -static ssize_t pointer_press_to_select_show(struct device *dev,
>> +static ssize_t attr_press_to_select_show_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->press_to_select);
>> }
>>
>> -static ssize_t pointer_press_to_select_store(struct device *dev,
>> +static ssize_t attr_press_to_select_store_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf,
>> size_t count)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> int value;
>>
>> if (kstrtoint(buf, 10, &value))
>> @@ -94,28 +93,28 @@ static ssize_t pointer_press_to_select_store(struct device *dev,
>> return -EINVAL;
>>
>> data_pointer->press_to_select = value;
>> - tpkbd_features_set(hdev);
>> + lenovo_features_set_tpkbd(hdev);
>>
>> return count;
>> }
>>
>> -static ssize_t pointer_dragging_show(struct device *dev,
>> +static ssize_t attr_dragging_show_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->dragging);
>> }
>>
>> -static ssize_t pointer_dragging_store(struct device *dev,
>> +static ssize_t attr_dragging_store_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf,
>> size_t count)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> int value;
>>
>> if (kstrtoint(buf, 10, &value))
>> @@ -124,28 +123,28 @@ static ssize_t pointer_dragging_store(struct device *dev,
>> return -EINVAL;
>>
>> data_pointer->dragging = value;
>> - tpkbd_features_set(hdev);
>> + lenovo_features_set_tpkbd(hdev);
>>
>> return count;
>> }
>>
>> -static ssize_t pointer_release_to_select_show(struct device *dev,
>> +static ssize_t attr_release_to_select_show_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->release_to_select);
>> }
>>
>> -static ssize_t pointer_release_to_select_store(struct device *dev,
>> +static ssize_t attr_release_to_select_store_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf,
>> size_t count)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> int value;
>>
>> if (kstrtoint(buf, 10, &value))
>> @@ -154,28 +153,28 @@ static ssize_t pointer_release_to_select_store(struct device *dev,
>> return -EINVAL;
>>
>> data_pointer->release_to_select = value;
>> - tpkbd_features_set(hdev);
>> + lenovo_features_set_tpkbd(hdev);
>>
>> return count;
>> }
>>
>> -static ssize_t pointer_select_right_show(struct device *dev,
>> +static ssize_t attr_select_right_show_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>> return snprintf(buf, PAGE_SIZE, "%u\n", data_pointer->select_right);
>> }
>>
>> -static ssize_t pointer_select_right_store(struct device *dev,
>> +static ssize_t attr_select_right_store_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf,
>> size_t count)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> int value;
>>
>> if (kstrtoint(buf, 10, &value))
>> @@ -184,119 +183,119 @@ static ssize_t pointer_select_right_store(struct device *dev,
>> return -EINVAL;
>>
>> data_pointer->select_right = value;
>> - tpkbd_features_set(hdev);
>> + lenovo_features_set_tpkbd(hdev);
>>
>> return count;
>> }
>>
>> -static ssize_t pointer_sensitivity_show(struct device *dev,
>> +static ssize_t attr_sensitivity_show_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>> return snprintf(buf, PAGE_SIZE, "%u\n",
>> data_pointer->sensitivity);
>> }
>>
>> -static ssize_t pointer_sensitivity_store(struct device *dev,
>> +static ssize_t attr_sensitivity_store_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf,
>> size_t count)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> int value;
>>
>> if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
>> return -EINVAL;
>>
>> data_pointer->sensitivity = value;
>> - tpkbd_features_set(hdev);
>> + lenovo_features_set_tpkbd(hdev);
>>
>> return count;
>> }
>>
>> -static ssize_t pointer_press_speed_show(struct device *dev,
>> +static ssize_t attr_press_speed_show_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>> return snprintf(buf, PAGE_SIZE, "%u\n",
>> data_pointer->press_speed);
>> }
>>
>> -static ssize_t pointer_press_speed_store(struct device *dev,
>> +static ssize_t attr_press_speed_store_tpkbd(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf,
>> size_t count)
>> {
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> int value;
>>
>> if (kstrtoint(buf, 10, &value) || value < 1 || value > 255)
>> return -EINVAL;
>>
>> data_pointer->press_speed = value;
>> - tpkbd_features_set(hdev);
>> + lenovo_features_set_tpkbd(hdev);
>>
>> return count;
>> }
>>
>> -static struct device_attribute dev_attr_pointer_press_to_select =
>> +static struct device_attribute dev_attr_press_to_select_tpkbd =
>> __ATTR(press_to_select, S_IWUSR | S_IRUGO,
>> - pointer_press_to_select_show,
>> - pointer_press_to_select_store);
>> + attr_press_to_select_show_tpkbd,
>> + attr_press_to_select_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_dragging =
>> +static struct device_attribute dev_attr_dragging_tpkbd =
>> __ATTR(dragging, S_IWUSR | S_IRUGO,
>> - pointer_dragging_show,
>> - pointer_dragging_store);
>> + attr_dragging_show_tpkbd,
>> + attr_dragging_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_release_to_select =
>> +static struct device_attribute dev_attr_release_to_select_tpkbd =
>> __ATTR(release_to_select, S_IWUSR | S_IRUGO,
>> - pointer_release_to_select_show,
>> - pointer_release_to_select_store);
>> + attr_release_to_select_show_tpkbd,
>> + attr_release_to_select_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_select_right =
>> +static struct device_attribute dev_attr_select_right_tpkbd =
>> __ATTR(select_right, S_IWUSR | S_IRUGO,
>> - pointer_select_right_show,
>> - pointer_select_right_store);
>> + attr_select_right_show_tpkbd,
>> + attr_select_right_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_sensitivity =
>> +static struct device_attribute dev_attr_sensitivity_tpkbd =
>> __ATTR(sensitivity, S_IWUSR | S_IRUGO,
>> - pointer_sensitivity_show,
>> - pointer_sensitivity_store);
>> + attr_sensitivity_show_tpkbd,
>> + attr_sensitivity_store_tpkbd);
>>
>> -static struct device_attribute dev_attr_pointer_press_speed =
>> +static struct device_attribute dev_attr_press_speed_tpkbd =
>> __ATTR(press_speed, S_IWUSR | S_IRUGO,
>> - pointer_press_speed_show,
>> - pointer_press_speed_store);
>> -
>> -static struct attribute *tpkbd_attributes_pointer[] = {
>> - &dev_attr_pointer_press_to_select.attr,
>> - &dev_attr_pointer_dragging.attr,
>> - &dev_attr_pointer_release_to_select.attr,
>> - &dev_attr_pointer_select_right.attr,
>> - &dev_attr_pointer_sensitivity.attr,
>> - &dev_attr_pointer_press_speed.attr,
>> + attr_press_speed_show_tpkbd,
>> + attr_press_speed_store_tpkbd);
>> +
>> +static struct attribute *lenovo_attributes_tpkbd[] = {
>> + &dev_attr_press_to_select_tpkbd.attr,
>> + &dev_attr_dragging_tpkbd.attr,
>> + &dev_attr_release_to_select_tpkbd.attr,
>> + &dev_attr_select_right_tpkbd.attr,
>> + &dev_attr_sensitivity_tpkbd.attr,
>> + &dev_attr_press_speed_tpkbd.attr,
>> NULL
>> };
>>
>> -static const struct attribute_group tpkbd_attr_group_pointer = {
>> - .attrs = tpkbd_attributes_pointer,
>> +static const struct attribute_group lenovo_attr_group_tpkbd = {
>> + .attrs = lenovo_attributes_tpkbd,
>> };
>>
>> -static enum led_brightness tpkbd_led_brightness_get(
>> +static enum led_brightness lenovo_led_brightness_get_tpkbd(
>> struct led_classdev *led_cdev)
>> {
>> struct device *dev = led_cdev->dev->parent;
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> int led_nr = 0;
>>
>> if (led_cdev == &data_pointer->led_micmute)
>> @@ -307,12 +306,12 @@ static enum led_brightness tpkbd_led_brightness_get(
>> : LED_OFF;
>> }
>>
>> -static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
>> +static void lenovo_led_brightness_set_tpkbd(struct led_classdev *led_cdev,
>> enum led_brightness value)
>> {
>> struct device *dev = led_cdev->dev->parent;
>> struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>> struct hid_report *report;
>> int led_nr = 0;
>>
>> @@ -330,10 +329,10 @@ static void tpkbd_led_brightness_set(struct led_classdev *led_cdev,
>> hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>> }
>>
>> -static int tpkbd_probe_tp(struct hid_device *hdev)
>> +static int lenovo_probe_tpkbd(struct hid_device *hdev)
>> {
>> struct device *dev = &hdev->dev;
>> - struct tpkbd_data_pointer *data_pointer;
>> + struct lenovo_drvdata_tpkbd *data_pointer;
>> size_t name_sz = strlen(dev_name(dev)) + 16;
>> char *name_mute, *name_micmute;
>> int i;
>> @@ -347,12 +346,12 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>> return -ENODEV;
>>
>> if (sysfs_create_group(&hdev->dev.kobj,
>> - &tpkbd_attr_group_pointer)) {
>> + &lenovo_attr_group_tpkbd)) {
>> hid_warn(hdev, "Could not create sysfs group\n");
>> }
>>
>> data_pointer = devm_kzalloc(&hdev->dev,
>> - sizeof(struct tpkbd_data_pointer),
>> + sizeof(struct lenovo_drvdata_tpkbd),
>> GFP_KERNEL);
>> if (data_pointer == NULL) {
>> hid_err(hdev, "Could not allocate memory for driver data\n");
>> @@ -375,23 +374,25 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>> hid_set_drvdata(hdev, data_pointer);
>>
>> data_pointer->led_mute.name = name_mute;
>> - data_pointer->led_mute.brightness_get = tpkbd_led_brightness_get;
>> - data_pointer->led_mute.brightness_set = tpkbd_led_brightness_set;
>> + data_pointer->led_mute.brightness_get = lenovo_led_brightness_get_tpkbd;
>> + data_pointer->led_mute.brightness_set = lenovo_led_brightness_set_tpkbd;
>> data_pointer->led_mute.dev = dev;
>> led_classdev_register(dev, &data_pointer->led_mute);
>>
>> data_pointer->led_micmute.name = name_micmute;
>> - data_pointer->led_micmute.brightness_get = tpkbd_led_brightness_get;
>> - data_pointer->led_micmute.brightness_set = tpkbd_led_brightness_set;
>> + data_pointer->led_micmute.brightness_get =
>> + lenovo_led_brightness_get_tpkbd;
>> + data_pointer->led_micmute.brightness_set =
>> + lenovo_led_brightness_set_tpkbd;
>> data_pointer->led_micmute.dev = dev;
>> led_classdev_register(dev, &data_pointer->led_micmute);
>>
>> - tpkbd_features_set(hdev);
>> + lenovo_features_set_tpkbd(hdev);
>>
>> return 0;
>> }
>>
>> -static int tpkbd_probe(struct hid_device *hdev,
>> +static int lenovo_probe(struct hid_device *hdev,
>> const struct hid_device_id *id)
>> {
>> int ret;
>> @@ -410,7 +411,7 @@ static int tpkbd_probe(struct hid_device *hdev,
>>
>> if (hid_get_drvdata(hdev)) {
>> hid_set_drvdata(hdev, NULL);
>> - ret = tpkbd_probe_tp(hdev);
>> + ret = lenovo_probe_tpkbd(hdev);
>> if (ret)
>> goto err_hid;
>> }
>> @@ -422,12 +423,12 @@ err:
>> return ret;
>> }
>>
>> -static void tpkbd_remove_tp(struct hid_device *hdev)
>> +static void lenovo_remove_tpkbd(struct hid_device *hdev)
>> {
>> - struct tpkbd_data_pointer *data_pointer = hid_get_drvdata(hdev);
>> + struct lenovo_drvdata_tpkbd *data_pointer = hid_get_drvdata(hdev);
>>
>> sysfs_remove_group(&hdev->dev.kobj,
>> - &tpkbd_attr_group_pointer);
>> + &lenovo_attr_group_tpkbd);
>>
>> led_classdev_unregister(&data_pointer->led_micmute);
>> led_classdev_unregister(&data_pointer->led_mute);
>> @@ -435,28 +436,28 @@ static void tpkbd_remove_tp(struct hid_device *hdev)
>> hid_set_drvdata(hdev, NULL);
>> }
>>
>> -static void tpkbd_remove(struct hid_device *hdev)
>> +static void lenovo_remove(struct hid_device *hdev)
>> {
>> if (hid_get_drvdata(hdev))
>> - tpkbd_remove_tp(hdev);
>> + lenovo_remove_tpkbd(hdev);
>>
>> hid_hw_stop(hdev);
>> }
>>
>> -static const struct hid_device_id tpkbd_devices[] = {
>> +static const struct hid_device_id lenovo_devices[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
>> { }
>> };
>>
>> -MODULE_DEVICE_TABLE(hid, tpkbd_devices);
>> +MODULE_DEVICE_TABLE(hid, lenovo_devices);
>>
>> -static struct hid_driver tpkbd_driver = {
>> - .name = "lenovo_tpkbd",
>> - .id_table = tpkbd_devices,
>> - .input_mapping = tpkbd_input_mapping,
>> - .probe = tpkbd_probe,
>> - .remove = tpkbd_remove,
>> +static struct hid_driver lenovo_driver = {
>> + .name = "lenovo",
>> + .id_table = lenovo_devices,
>> + .input_mapping = lenovo_input_mapping_tpkbd,
>> + .probe = lenovo_probe,
>> + .remove = lenovo_remove,
>> };
>> -module_hid_driver(tpkbd_driver);
>> +module_hid_driver(lenovo_driver);
>>
>> MODULE_LICENSE("GPL");
>> --
>> 2.0.0
>>
>
>
>
--
Jamie Lentin
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Chen Gang @ 2014-07-18 10:44 UTC (permalink / raw)
To: Richard Weinberger, Arnd Bergmann, Lars-Peter Clausen
Cc: Guenter Roeck, Greg Kroah-Hartman, dmitry.torokhov, linux-iio,
Benjamin Herrenschmidt, teg, Thierry Reding, Lennox Wu,
Marek Vasut, Liqin Chen, msalter, linux-pwm, devel,
linux-watchdog, linux-input, linux-kernel@vger.kernel.org,
knaack.h, Martin Schwidefsky, Mischa.Jonker, jic23,
Geert Uytterhoeven
In-Reply-To: <53C8CE59.3060103@nod.at>
On 07/18/2014 03:35 PM, Richard Weinberger wrote:
> Am 18.07.2014 02:36, schrieb Chen Gang:
>>
>> On 07/18/2014 02:09 AM, Richard Weinberger wrote:
>>> Am 17.07.2014 12:48, schrieb Arnd Bergmann:
>>>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>>>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>>>> to build on UML seems pointless to me and we special-case it in a number of
>>>> places already.
>>>
>>> If UML is the only arch without io memory the dependency on !UML seems
>>> reasonable to me. :-)
>>>
>>
>> For me, if only uml left, I suggest to implement dummy functions within
>> uml instead of let CONFIG_UML appear in generic include directory. And
>> then remove all HAS_IOMEM and NO_IOMEM from kernel.
>
> Erm, this is something completely different.
> I thought we're focusing on COMPILE_TEST?
>
COMPILE_TEST is none-architecture specific, but UML is. So in generic
include folder, if we're focusing on choosing whether COMPILE_TEST or
UML, for me, I will choose COMPILE_TEST.
If we're not only focusing on COMPILE_TEST, for me, if something only
depend on one architecture, I'd like to put them under "arch/*/" folder.
Especially, after that, we can remove all HAS_IOMEM and NO_IOMEM, nobody
has to think of them again. :-)
Thanks.
--
Chen Gang
Open, share, and attitude like air, water, and life which God blessed
^ permalink raw reply
* Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'
From: Richard Weinberger @ 2014-07-18 10:51 UTC (permalink / raw)
To: Chen Gang, Arnd Bergmann, Lars-Peter Clausen
Cc: Guenter Roeck, Greg Kroah-Hartman, dmitry.torokhov, linux-iio,
Benjamin Herrenschmidt, teg, Thierry Reding, Lennox Wu,
Marek Vasut, Liqin Chen, msalter, linux-pwm, devel,
linux-watchdog, linux-input, linux-kernel@vger.kernel.org,
knaack.h, Martin Schwidefsky, Mischa.Jonker, jic23,
Geert Uytterhoeven
In-Reply-To: <53C8FA78.1020501@gmail.com>
Am 18.07.2014 12:44, schrieb Chen Gang:
> On 07/18/2014 03:35 PM, Richard Weinberger wrote:
>> Am 18.07.2014 02:36, schrieb Chen Gang:
>>>
>>> On 07/18/2014 02:09 AM, Richard Weinberger wrote:
>>>> Am 17.07.2014 12:48, schrieb Arnd Bergmann:
>>>>> AFAICT, NO_IOMEM only has a real purpose on UML these days. Could we take
>>>>> a shortcut here and make COMPILE_TEST depend on !UML? Getting random stuff
>>>>> to build on UML seems pointless to me and we special-case it in a number of
>>>>> places already.
>>>>
>>>> If UML is the only arch without io memory the dependency on !UML seems
>>>> reasonable to me. :-)
>>>>
>>>
>>> For me, if only uml left, I suggest to implement dummy functions within
>>> uml instead of let CONFIG_UML appear in generic include directory. And
>>> then remove all HAS_IOMEM and NO_IOMEM from kernel.
>>
>> Erm, this is something completely different.
>> I thought we're focusing on COMPILE_TEST?
>>
>
> COMPILE_TEST is none-architecture specific, but UML is. So in generic
> include folder, if we're focusing on choosing whether COMPILE_TEST or
> UML, for me, I will choose COMPILE_TEST.
>
> If we're not only focusing on COMPILE_TEST, for me, if something only
> depend on one architecture, I'd like to put them under "arch/*/" folder.
>
> Especially, after that, we can remove all HAS_IOMEM and NO_IOMEM, nobody
> has to think of them again. :-)
And then we end up with a solution that on UML a lot of completely useless
drivers are build which fail in various interesting manners because you'll
add stubs for all kinds of io memory related functions to arch/um/?
We had this kind of discussion already. You'll need more than ioremap...
I like Arnd's idea *much* more to make COMPILE_TEST depend on !UML.
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH v2] HID: rmi: check that report ids exist in the report_id_hash before accessing their size
From: Benjamin Tissoires @ 2014-07-18 13:03 UTC (permalink / raw)
To: Andrew Duggan; +Cc: linux-input, linux-kernel, Jiri Kosina
In-Reply-To: <1405638885-8014-1-git-send-email-aduggan@synaptics.com>
On Jul 17 2014 or thereabouts, Andrew Duggan wrote:
> It is possible that the hid-rmi driver could get loaded onto a device which does not have the
> expected report ids. This should not happen because it would indicate that the hid-rmi driver is
> not compatible with that device. However, if it does happen it should return an error from probe
> instead of dereferencing a null pointer.
>
> related bug:
> https://bugzilla.kernel.org/show_bug.cgi?id=80091
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> ---
> Added hid_err messages as suggested by Benjamin. This fixes the null pointer dereference from
> bug 80091 while the next patch addresses the binding issue in hid-core.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
>
> drivers/hid/hid-rmi.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 3221a95..260be7a 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -848,6 +848,8 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
> struct rmi_data *data = NULL;
> int ret;
> size_t alloc_size;
> + struct hid_report *input_report;
> + struct hid_report *output_report;
>
> data = devm_kzalloc(&hdev->dev, sizeof(struct rmi_data), GFP_KERNEL);
> if (!data)
> @@ -866,12 +868,26 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> - data->input_report_size = (hdev->report_enum[HID_INPUT_REPORT]
> - .report_id_hash[RMI_ATTN_REPORT_ID]->size >> 3)
> - + 1 /* report id */;
> - data->output_report_size = (hdev->report_enum[HID_OUTPUT_REPORT]
> - .report_id_hash[RMI_WRITE_REPORT_ID]->size >> 3)
> - + 1 /* report id */;
> + input_report = hdev->report_enum[HID_INPUT_REPORT]
> + .report_id_hash[RMI_ATTN_REPORT_ID];
> + if (!input_report) {
> + hid_err(hdev, "device does not have expected input report\n");
> + ret = -ENODEV;
> + return ret;
> + }
> +
> + data->input_report_size = (input_report->size >> 3) + 1 /* report id */;
> +
> + output_report = hdev->report_enum[HID_OUTPUT_REPORT]
> + .report_id_hash[RMI_WRITE_REPORT_ID];
> + if (!output_report) {
> + hid_err(hdev, "device does not have expected output report\n");
> + ret = -ENODEV;
> + return ret;
> + }
> +
> + data->output_report_size = (output_report->size >> 3)
> + + 1 /* report id */;
>
> alloc_size = data->output_report_size + data->input_report_size;
>
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH] HID: rmi: only bind the hid-rmi driver to the mouse interface of composite USB devices
From: Benjamin Tissoires @ 2014-07-18 13:07 UTC (permalink / raw)
To: Andrew Duggan; +Cc: linux-input, linux-kernel, Jiri Kosina
In-Reply-To: <1405638885-8014-2-git-send-email-aduggan@synaptics.com>
On Jul 17 2014 or thereabouts, Andrew Duggan wrote:
> On composite HID devices there may be multiple HID devices on separate interfaces, but hid-rmi
> should only bind to the mouse interface. One example is the Dell Venue 11 Pro's keyboard dock
> which contains a composite USB device with a HID touchpad and HID keyboard on separate intefaces.
> Since the USB Vendor ID is Synaptic's, hid-core is currently trying to bind hid-rmi to all\of
> the HID devices. This patch ensures that hid-rmi only binds to the mouse interface.
>
> related bug:
> https://bugzilla.kernel.org/show_bug.cgi?id=80091
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> ---
As mentioned in a private mail, Andrew told me that for now, each
touchpad using hid-rmi he has seen are using the boot mouse protocol, so
this patch will give us some space and will not break existing current
setups.
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> drivers/hid/hid-core.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 8ed66fd..f10e768 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -783,7 +783,9 @@ static int hid_scan_report(struct hid_device *hid)
> * Vendor specific handlings
> */
> if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
> - (hid->group == HID_GROUP_GENERIC))
> + (hid->group == HID_GROUP_GENERIC) &&
> + /* only bind to the mouse interface of composite USB devices */
> + (hid->bus != BUS_USB || hid->type == HID_TYPE_USBMOUSE))
> /* hid-rmi should take care of them, not hid-generic */
> hid->group = HID_GROUP_RMI;
>
> --
> 1.9.1
>
^ permalink raw reply
* Re: Power-managing devices that are not of interest at some point in time
From: Alan Stern @ 2014-07-18 15:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Dmitry Torokhov, Bastien Nocera, Patrik Fimml, linux-pm,
Benson Leung, linux-input, linux-kernel
In-Reply-To: <2674541.jV2qHCZlHf@vostro.rjw.lan>
On Fri, 18 Jul 2014, Rafael J. Wysocki wrote:
> > > the problem really seems to be that drivers are not
> > > aggressive enough with starting PM transitions (using runtime PM) when they
> > > see no activity. Thus it seems that when the lid is closed, it'll be good
> > > to switch the drivers into a "more aggressive runtime PM mode" in which
> > > they will use any opportunity to start a power transition without worrying
> > > about extra latencies resulting from that. In that mode they should also
> > > disable remote wakeup. I think this should be sufficient to address the
> > > use case at hand.
> >
> > OK, so how do we let the drivers know that they should start being aggressive
> > with PM and that they should disable remote wakeup? I'd rather not add
> > subsystem-specific interface for this, that is why we are reaching out in the
> > first place.
>
> For disabling remote wakeup we have a PM QoS flag that I'm not entirely happy
> with, so I guess we can replace it with something saner (I talked about that
> with Alan during the last year's LinuxCon, but then didn't have the time to
> get to that).
Right. We discussed changing .../power/wakeup so that it could contain
"enabled", "disabled", or "off", where "off" would mean remote wakeup
should be disabled even during runtime suspend.
> For the whole thing I guess we can add a sysfs attribute under devices/.../power
> that will need to be exposed by drivers supporting that feature. I'm not sure
> how to call it, though.
"runtime_mode"?
Will this really be capable of doing what Dmitry wants? I don't know
how the drivers in question work. But if a driver increments the
runtime usage count each time the device file is opened, forcibly
setting the usage count back to 0 won't be easy.
Also, would putting the camera into runtime suspend prevent it from
showing up on a list of available video devices? I doubt it. More
likely, the video driver would have to unregister the class device
while remaining bound to the physical device. Probably other drivers
would have to do the same sort of thing. (I don't know whether doing
this would retain the settings as Oliver wants, though.)
Alan Stern
^ permalink raw reply
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