* [PATCH v1 2/5] IB/uverbs: ex_query_device: check request's comp_mask
From: Yann Droneaud @ 2015-01-29 17:59 UTC (permalink / raw)
To: Sagi Grimberg, Shachar Raindel, Eli Cohen, Haggai Eran,
Roland Dreier
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1422553023.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
This patch ensures the extended QUERY_DEVICE uverbs request's
comp_mask has only known and supported bits (currently none).
If userspace set unknown features bits, -EINVAL will be returned,
ensuring current programs are not allowed to set random feature
bits: such bits could enable new extended features in future kernel
versions and those features can trigger a behavior not unsupported
by the older programs or make the newer kernels return an error
for a request which was valid on older kernels.
Additionally, returning an error for unsupported feature would
allow userspace to probe/discover which extended features are
currently supported by a kernel.
Link: http://mid.gmane.org/cover.1422553023.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
drivers/infiniband/core/uverbs_cmd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 6ef06a9b4362..fbcc54b86795 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3312,6 +3312,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
if (err)
return err;
+ if (cmd.comp_mask)
+ return -EINVAL;
+
if (cmd.reserved)
return -EINVAL;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v1 1/5] IB/uverbs: ex_query_device: answer must not depend on request's comp_mask
From: Yann Droneaud @ 2015-01-29 17:59 UTC (permalink / raw)
To: Sagi Grimberg, Shachar Raindel, Eli Cohen, Haggai Eran,
Roland Dreier
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <cover.1422553023.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
As specified in "Extending Verbs API" presentation [1] by Tzahi Oved
during OFA International Developer Workshop 2013, the request's
comp_mask should describe the request data: it's describe the
availability of extended fields in the request.
Conversely, the response's comp_mask should describe the presence
of extended fields in the response.
So this patch changes ib_uverbs_ex_query_device() function to always
return the maximum supported features, currently only
IB_USER_VERBS_EX_QUERY_DEVICE_ODP per commit 860f10a799c8 ("IB/core:
Add flags for on demand paging support"), regardless of the value of
request's comp_mask.
[1] https://www.openfabrics.org/images/docs/2013_Dev_Workshop/Tues_0423/2013_Workshop_Tues_0830_Tzahi_Oved-verbs_extensions_ofa_2013-tzahio.pdf
Link: http://mid.gmane.org/cover.1422553023.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
drivers/infiniband/core/uverbs_cmd.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 532d8eba8b02..6ef06a9b4362 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -3324,17 +3324,15 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
resp.comp_mask = 0;
#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
- if (cmd.comp_mask & IB_USER_VERBS_EX_QUERY_DEVICE_ODP) {
- resp.odp_caps.general_caps = attr.odp_caps.general_caps;
- resp.odp_caps.per_transport_caps.rc_odp_caps =
- attr.odp_caps.per_transport_caps.rc_odp_caps;
- resp.odp_caps.per_transport_caps.uc_odp_caps =
- attr.odp_caps.per_transport_caps.uc_odp_caps;
- resp.odp_caps.per_transport_caps.ud_odp_caps =
- attr.odp_caps.per_transport_caps.ud_odp_caps;
- resp.comp_mask |= IB_USER_VERBS_EX_QUERY_DEVICE_ODP;
- }
+ resp.odp_caps.general_caps = attr.odp_caps.general_caps;
+ resp.odp_caps.per_transport_caps.rc_odp_caps =
+ attr.odp_caps.per_transport_caps.rc_odp_caps;
+ resp.odp_caps.per_transport_caps.uc_odp_caps =
+ attr.odp_caps.per_transport_caps.uc_odp_caps;
+ resp.odp_caps.per_transport_caps.ud_odp_caps =
+ attr.odp_caps.per_transport_caps.ud_odp_caps;
#endif
+ resp.comp_mask |= IB_USER_VERBS_EX_QUERY_DEVICE_ODP;
err = ib_copy_to_udata(ucore, &resp, sizeof(resp));
if (err)
--
2.1.0
^ permalink raw reply related
* [PATCH v1 0/5] IB/core: extended query device caps cleanup for v3.19
From: Yann Droneaud @ 2015-01-29 17:59 UTC (permalink / raw)
To: Sagi Grimberg, Shachar Raindel, Eli Cohen, Haggai Eran,
Roland Dreier
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Hi,
Following discussions in thread "[PATCH v3 06/17] IB/core: Add support
for extended query device caps" [1] and further comments on previous
patch in "[PATCH 3/4] IB/uverbs: ex_query_device: check request's
comp_mask" thread [2], I'm proposing an updated patchset to implement
a slighly different behavior to handle the request parameters in
ib_uverbs_ex_query_device() in order to restore the current behavior
of ib_copy_to_udata().
I think we found an agreement on the scheme to be implemented in
Haggai Eran's response [3]:
- input's comp_mask is currently not used, so it must be 0;
- the response buffer size is checked so that the base answer
can be returned without being truncated;
- the extended feature (odp) information are returned
if, and only if, there's enough space in the response buffer,
allowing older programs to get only the features they would
expect. Newer programs are expected to provide more space,
but as the response's comp_mask will describe only the available
fields, newer program cannot be surprized to not get information
when run on older kernel (if we want to support this use case).
I feel even more confident this behavior would allow a better
maintainability. Additionally, it's still looking more like the
behavior already implemented by other InfiniBand/RDMA kernel <->
userspace interfaces.
I hope this would go in v3.19 before its release so that the extended
QUERY_DEVICE uverbs won't hurt us being part of an official release:
in it's current shape it won't be easy to support it.
Regards.
Changes from v0 [4]
- don't use input's comp_mask to conditionnaly build the response
- ensure input's comp_mask is set 0
[1] http://mid.gmane.org/1418733236.2779.26.camel-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[2] http://mid.gmane.org/063956366559d6919693aabb324bab83d676bc28.1421931555.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[3] http://mid.gmane.org/54C902E4.5010600-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
[4] http://mid.gmane.org/cover.1421931555.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Yann Droneaud (5):
IB/uverbs: ex_query_device: answer must not depend on request's
comp_mask
IB/uverbs: ex_query_device: check request's comp_mask
IB/uverbs: ex_query_device: answer must depend on response buffer's
size
IB/uverbs: ex_query_device: no need to clear the whole structure
IB/core: ib_copy_to_udata(): don't silently truncate response
drivers/infiniband/core/uverbs_cmd.c | 39 +++++++++++++++++++++++++-----------
include/rdma/ib_verbs.h | 5 +----
2 files changed, 28 insertions(+), 16 deletions(-)
--
2.1.0
^ permalink raw reply
* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
From: Sören Brinkmann @ 2015-01-29 17:23 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Courbot, Johan Hovold, linux-api,
Linux Kernel Mailing List, linux-gpio@vger.kernel.org,
linux-doc@vger.kernel.org
In-Reply-To: <CACRpkdZ4CekD2xeJYM+UUtuGuwS2uB++XprSueWnshEVk=79Fw@mail.gmail.com>
Hi Linus,
On Mon, 2015-01-19 at 09:54AM +0100, Linus Walleij wrote:
> On Mon, Jan 19, 2015 at 5:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Sat, Jan 17, 2015 at 1:49 AM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> >> On Fri, 2015-01-16 at 12:11PM +0100, Johan Hovold wrote:
>
> >>> Implementing proper wakeup support for unclaimed GPIOs would take some
> >>> work (if at all desired), but that is not a reason to be adding custom
> >>> implementations that violates the kernel's power policies and new ABIs
> >>> that would need to be maintained forever.
> (...)
> >>> Meanwhile you can (should) use gpio-keys if you need to wake your system
> >>> on gpio events.
> >>
> >> We had that discussion and I don't think GPIO keys is the right solution
> >> for every use-case.
> >
> > Sorry, it has been a while - can you remind us of why?
>
> There are such cases. Of course keys should be handled by GPIO-keys
> and these will trigger the right wakeup events in such cases.
>
> This is for more esoteric cases: we cannot have a kernel module for
> everything people want to do with GPIOs, and the use case I accept
> is GPIOs used in automatic control etc, think factory lines or doors.
> We can't have a "door" driver or "punch arm" or "fire alarm" driver
> in the kernel. Those are userspace things.
>
> Still such embedded systems need to be able to go to idle and
> sleep to conerve power, and then they need to put wakeups on
> these GPIOs.
>
> So it is a feature userspace needs, though as with much of the
> sysfs ABI it is very often abused for things like keys and LEDs which
> is an abomination but we can't do much about it :(
Thanks for clearing that up.
What does that mean for this patch? Are we going ahead, accepting the
extension of this API or do all these use-cases have to wait for the
rewrite of a proper GPIO userspace interface?
Thanks,
Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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 v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Peter Hurley @ 2015-01-29 16:34 UTC (permalink / raw)
To: Russell King - ARM Linux, Thierry Reding
Cc: Varka Bhadram, Chunyan Zhang,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, mark.rutland-5wv7dgnIgG8,
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io,
heiko-4mtYJXux2i+zQB+pC5nmwQ, andrew-g2DYL2Zd6BY,
jslaby-AlSwsSmVLrQ, lanqing.liu-lxIno14LUO0EEoCn2XhGlw,
pawel.moll-5wv7dgnIgG8, zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w,
zhizhou.zhang-lxIno14LUO0EEoCn2XhGlw,
geng.ren-lxIno14LUO0EEoCn2XhGlw,
antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
orsonzhai-Re5JQEeQqe8AvxtiuMwx3w, florian.vaussard-p8DiymsW2f8,
devicetree-u79uwXL29TY76Z2rM5mHXA, jason-NLaQJdtUoK4Be96aLqz0jA,
arnd-r2nGTMty4D4, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
hytszk-Re5JQEeQqe8AvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
wei.qiao-lxIno14LUO0EEoCn2XhGlw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, galak-sgV2jX0FEOL9JmXXK+q4OQ,
shawn.guo-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <20150129160553.GC26493-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>
On 01/29/2015 11:05 AM, Russell King - ARM Linux wrote:
> On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote:
>> Hi Varka,
>>
>> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
>>> This check is not required. It will be done by devm_ioremap_resource()
>>
>> I disagree. devm_ioremap_resource() interprets the NULL resource as
>> a bad parameter and returns -EINVAL which is then forwarded as the
>> return value from the probe.
>>
>> -ENODEV is the correct return value from the probe if the expected
>> resource is not available (either because it doesn't exist or was already
>> claimed by another driver).
>
> (Adding Thierry as the author of this function.)
>
> I believe devm_ioremap_resource() was explicitly designed to remove such
> error handling from drivers, and to give drivers a unified error response
> to such things as missing resources.
>
> See the comments for this function in lib/devres.c.
Maybe the purpose would be better served by wrapping the idiom in a single
function.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [RESEND] [PATCH] block: create ioctl to discard-or-zeroout a range of blocks
From: Elliott, Robert (Server Storage) @ 2015-01-29 16:28 UTC (permalink / raw)
To: Arnd Bergmann, Darrick J. Wong
Cc: Jens Axboe, Christoph Hellwig,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jeff Layton,
J. Bruce Fields
In-Reply-To: <2747023.BlTyJ4fNVf@wuerfel>
> -----Original Message-----
> From: linux-kernel-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-kernel-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Arnd Bergmann
> Sent: Thursday, 29 January, 2015 4:03 AM
> To: Darrick J. Wong
> Cc: Jens Axboe; Christoph Hellwig; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jeff Layton; J. Bruce
> Fields
> Subject: Re: [RESEND] [PATCH] block: create ioctl to discard-or-zeroout a
> range of blocks
>
> On Wednesday 28 January 2015 18:00:25 Darrick J. Wong wrote:
> > Create a new ioctl to expose the block layer's newfound ability to
> > issue either a zeroing discard, a WRITE SAME with a zero page, or a
> > regular write with the zero page. This BLKZEROOUT2 ioctl takes
> > {start, length, flags} as parameters. So far, the only flag available
> > is to enable the zeroing discard part -- without it, the call invokes
> > the old BLKZEROOUT behavior. start and length have the same meaning
> > as in BLKZEROOUT.
> >
> > Furthermore, because BLKZEROOUT2 issues commands directly to the
> > storage device, we must invalidate the page cache (as a regular
> > O_DIRECT write would do) to avoid returning stale cache contents at a
> > later time.
> >
> > This patch depends on "block: Add discard flag to
> > blkdev_issue_zeroout() function" in Jens' for-3.20/core branch.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> >
>
> Would this work ok for devices that fill discarded areas with all-ones
> instead of all-zeroes? I believe SD cards can do either.
>
> Arnd
RAID software stacks need predictable data, and 0 is a lot easier to
deal with than 0xff in Galois field (XOR-based) math.
The SCSI and ATA standards both define bits to report if the device is
designed to return 0 after discard; they don't have a way to report
that any other particular non-zero value is returned.
---
Rob Elliott HP Server Storage
^ permalink raw reply
* Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Peter Hurley @ 2015-01-29 16:16 UTC (permalink / raw)
To: Varka Bhadram, Chunyan Zhang,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
arnd-r2nGTMty4D4, gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io,
pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
jslaby-AlSwsSmVLrQ, heiko-4mtYJXux2i+zQB+pC5nmwQ,
jason-NLaQJdtUoK4Be96aLqz0jA, florian.vaussard-p8DiymsW2f8,
andrew-g2DYL2Zd6BY, hytszk-Re5JQEeQqe8AvxtiuMwx3w,
antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w,
shawn.guo-QSEj5FYQhm4dnm+yROfE0A,
orsonzhai-Re5JQEeQqe8AvxtiuMwx3w, geng.ren-lxIno14LUO0EEoCn2XhGlw,
zhizhou.zhang-lxIno14LUO0EEoCn2XhGlw,
lanqing.liu-lxIno14LUO0EEoCn2XhGlw,
zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w,
wei.qiao-lxIno14LUO0EEoCn2XhGlw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <54CA59F1.3060008-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 01/29/2015 11:04 AM, Varka Bhadram wrote:
> Hi Peter,
>
> On Thursday 29 January 2015 09:19 PM, Peter Hurley wrote:
>> Hi Varka,
>>
>> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
>>> Hi,
>>>
>>> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>>>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>>>> spreadtrum sharkl64 platform.
>>>> This driver also support earlycon.
>>>>
>>>> Originally-by: Lanqing Liu <lanqing.liu-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>>>> Signed-off-by: Orson Zhai <orson.zhai-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>>>> Signed-off-by: Chunyan Zhang <chunyan.zhang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>>>> Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>>>> Reviewed-by: Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>
>>>> ---
>>>> drivers/tty/serial/Kconfig | 18 +
>>>> drivers/tty/serial/Makefile | 1 +
>>>> drivers/tty/serial/sprd_serial.c | 793 ++++++++++++++++++++++++++++++++++++++
>>>> include/uapi/linux/serial_core.h | 3 +
>>>> 4 files changed, 815 insertions(+)
>>>> create mode 100644 drivers/tty/serial/sprd_serial.c
>>>>
>>> (...)
>>>
>>>> +static int sprd_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct resource *res;
>>>> + struct uart_port *up;
>>>> + struct clk *clk;
>>>> + int irq;
>>>> + int index;
>>>> + int ret;
>>>> +
>>>> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>>>> + if (sprd_port[index] == NULL)
>>>> + break;
>>>> +
>>>> + if (index == ARRAY_SIZE(sprd_port))
>>>> + return -EBUSY;
>>>> +
>>>> + index = sprd_probe_dt_alias(index, &pdev->dev);
>>>> +
>>>> + sprd_port[index] = devm_kzalloc(&pdev->dev,
>>>> + sizeof(*sprd_port[index]), GFP_KERNEL);
>>>> + if (!sprd_port[index])
>>>> + return -ENOMEM;
>>>> +
>>>> + up = &sprd_port[index]->port;
>>>> + up->dev = &pdev->dev;
>>>> + up->line = index;
>>>> + up->type = PORT_SPRD;
>>>> + up->iotype = SERIAL_IO_PORT;
>>>> + up->uartclk = SPRD_DEF_RATE;
>>>> + up->fifosize = SPRD_FIFO_SIZE;
>>>> + up->ops = &serial_sprd_ops;
>>>> + up->flags = UPF_BOOT_AUTOCONF;
>>>> +
>>>> + clk = devm_clk_get(&pdev->dev, NULL);
>>>> + if (!IS_ERR(clk))
>>>> + up->uartclk = clk_get_rate(clk);
>>>> +
>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + if (!res) {
>>>> + dev_err(&pdev->dev, "not provide mem resource\n");
>>>> + return -ENODEV;
>>>> + }
>>> This check is not required. It will be done by devm_ioremap_resource()
>> I disagree. devm_ioremap_resource() interprets the NULL resource as
>> a bad parameter and returns -EINVAL which is then forwarded as the
>> return value from the probe.
>>
>> -ENODEV is the correct return value from the probe if the expected
>> resource is not available (either because it doesn't exist or was already
>> claimed by another driver).
>
> Check on the resource happening with evm_ioremap_resource.
>
> Not necessary to check multiple times.
>
> I did series for all the drivers. see [1]
>
> [1]: https://lkml.org/lkml/2014/11/3/986 <https://lkml.org/lkml/2014/11/3/986>
That's a usb series. Did you do a serial driver series I missed?
I don't see anything related in Greg's tty-next tree...
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Russell King - ARM Linux @ 2015-01-29 16:05 UTC (permalink / raw)
To: Peter Hurley, Thierry Reding
Cc: Varka Bhadram, Chunyan Zhang,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, mark.rutland-5wv7dgnIgG8,
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io,
heiko-4mtYJXux2i+zQB+pC5nmwQ, andrew-g2DYL2Zd6BY,
jslaby-AlSwsSmVLrQ, lanqing.liu-lxIno14LUO0EEoCn2XhGlw,
pawel.moll-5wv7dgnIgG8, zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w,
zhizhou.zhang-lxIno14LUO0EEoCn2XhGlw,
geng.ren-lxIno14LUO0EEoCn2XhGlw,
antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
orsonzhai-Re5JQEeQqe8AvxtiuMwx3w, florian.vaussard-p8DiymsW2f8,
devicetree-u79uwXL29TY76Z2rM5mHXA, jason-NLaQJdtUoK4Be96aLqz0jA,
arnd-r2nGTMty4D4, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
hytszk-Re5JQEeQqe8AvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
wei.qiao-lxIno14LUO0EEoCn2XhGlw,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, galak-sgV2jX0FEOL9JmXXK+q4OQ,
shawn.guo-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <54CA568E.6080306-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>
On Thu, Jan 29, 2015 at 10:49:34AM -0500, Peter Hurley wrote:
> Hi Varka,
>
> On 01/29/2015 10:26 AM, Varka Bhadram wrote:
> > This check is not required. It will be done by devm_ioremap_resource()
>
> I disagree. devm_ioremap_resource() interprets the NULL resource as
> a bad parameter and returns -EINVAL which is then forwarded as the
> return value from the probe.
>
> -ENODEV is the correct return value from the probe if the expected
> resource is not available (either because it doesn't exist or was already
> claimed by another driver).
(Adding Thierry as the author of this function.)
I believe devm_ioremap_resource() was explicitly designed to remove such
error handling from drivers, and to give drivers a unified error response
to such things as missing resources.
See the comments for this function in lib/devres.c.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Peter Hurley @ 2015-01-29 15:49 UTC (permalink / raw)
To: Varka Bhadram, Chunyan Zhang,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
arnd-r2nGTMty4D4, gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io,
pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
jslaby-AlSwsSmVLrQ, heiko-4mtYJXux2i+zQB+pC5nmwQ,
jason-NLaQJdtUoK4Be96aLqz0jA, florian.vaussard-p8DiymsW2f8,
andrew-g2DYL2Zd6BY, hytszk-Re5JQEeQqe8AvxtiuMwx3w,
antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w,
shawn.guo-QSEj5FYQhm4dnm+yROfE0A,
orsonzhai-Re5JQEeQqe8AvxtiuMwx3w, geng.ren-lxIno14LUO0EEoCn2XhGlw,
zhizhou.zhang-lxIno14LUO0EEoCn2XhGlw,
lanqing.liu-lxIno14LUO0EEoCn2XhGlw,
zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w,
wei.qiao-lxIno14LUO0EEoCn2XhGlw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <54CA5128.8050500-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Varka,
On 01/29/2015 10:26 AM, Varka Bhadram wrote:
> Hi,
>
> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>> spreadtrum sharkl64 platform.
>> This driver also support earlycon.
>>
>> Originally-by: Lanqing Liu <lanqing.liu-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>> Signed-off-by: Orson Zhai <orson.zhai-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>> Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> Reviewed-by: Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>
>> ---
>> drivers/tty/serial/Kconfig | 18 +
>> drivers/tty/serial/Makefile | 1 +
>> drivers/tty/serial/sprd_serial.c | 793 ++++++++++++++++++++++++++++++++++++++
>> include/uapi/linux/serial_core.h | 3 +
>> 4 files changed, 815 insertions(+)
>> create mode 100644 drivers/tty/serial/sprd_serial.c
>>
> (...)
>
>> +static int sprd_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + struct uart_port *up;
>> + struct clk *clk;
>> + int irq;
>> + int index;
>> + int ret;
>> +
>> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>> + if (sprd_port[index] == NULL)
>> + break;
>> +
>> + if (index == ARRAY_SIZE(sprd_port))
>> + return -EBUSY;
>> +
>> + index = sprd_probe_dt_alias(index, &pdev->dev);
>> +
>> + sprd_port[index] = devm_kzalloc(&pdev->dev,
>> + sizeof(*sprd_port[index]), GFP_KERNEL);
>> + if (!sprd_port[index])
>> + return -ENOMEM;
>> +
>> + up = &sprd_port[index]->port;
>> + up->dev = &pdev->dev;
>> + up->line = index;
>> + up->type = PORT_SPRD;
>> + up->iotype = SERIAL_IO_PORT;
>> + up->uartclk = SPRD_DEF_RATE;
>> + up->fifosize = SPRD_FIFO_SIZE;
>> + up->ops = &serial_sprd_ops;
>> + up->flags = UPF_BOOT_AUTOCONF;
>> +
>> + clk = devm_clk_get(&pdev->dev, NULL);
>> + if (!IS_ERR(clk))
>> + up->uartclk = clk_get_rate(clk);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev, "not provide mem resource\n");
>> + return -ENODEV;
>> + }
>
> This check is not required. It will be done by devm_ioremap_resource()
I disagree. devm_ioremap_resource() interprets the NULL resource as
a bad parameter and returns -EINVAL which is then forwarded as the
return value from the probe.
-ENODEV is the correct return value from the probe if the expected
resource is not available (either because it doesn't exist or was already
claimed by another driver).
Regards,
Peter Hurley
>> + up->mapbase = res->start;
>> + up->membase = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(up->membase))
>> + return PTR_ERR(up->membase);
>> +
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Varka Bhadram @ 2015-01-29 15:32 UTC (permalink / raw)
To: Chunyan Zhang, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
arnd-r2nGTMty4D4, gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io,
peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8, pawel.moll-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, grant.likely-QSEj5FYQhm4dnm+yROfE0A,
jslaby-AlSwsSmVLrQ, heiko-4mtYJXux2i+zQB+pC5nmwQ,
jason-NLaQJdtUoK4Be96aLqz0jA, florian.vaussard-p8DiymsW2f8,
andrew-g2DYL2Zd6BY, hytszk-Re5JQEeQqe8AvxtiuMwx3w,
antonynpavlov-Re5JQEeQqe8AvxtiuMwx3w,
shawn.guo-QSEj5FYQhm4dnm+yROfE0A,
orsonzhai-Re5JQEeQqe8AvxtiuMwx3w, geng.ren-lxIno14LUO0EEoCn2XhGlw,
zhizhou.zhang-lxIno14LUO0EEoCn2XhGlw,
lanqing.liu-lxIno14LUO0EEoCn2XhGlw,
zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w,
wei.qiao-lxIno14LUO0EEoCn2XhGlw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <54CA5128.8050500-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thursday 29 January 2015 08:56 PM, Varka Bhadram wrote:
> Hi,
>
> On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
>> Add a full sc9836-uart driver for SC9836 SoC which is based on the
>> spreadtrum sharkl64 platform.
>> This driver also support earlycon.
>>
>> Originally-by: Lanqing Liu <lanqing.liu-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>> Signed-off-by: Orson Zhai <orson.zhai-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>> Signed-off-by: Chunyan Zhang <chunyan.zhang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>> Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> Reviewed-by: Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>
>> ---
>> drivers/tty/serial/Kconfig | 18 +
>> drivers/tty/serial/Makefile | 1 +
>> drivers/tty/serial/sprd_serial.c | 793
>> ++++++++++++++++++++++++++++++++++++++
>> include/uapi/linux/serial_core.h | 3 +
>> 4 files changed, 815 insertions(+)
>> create mode 100644 drivers/tty/serial/sprd_serial.c
>>
> (...)
>
>> +static int sprd_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + struct uart_port *up;
>> + struct clk *clk;
>> + int irq;
>> + int index;
>> + int ret;
>> +
>> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
>> + if (sprd_port[index] == NULL)
>> + break;
>> +
>> + if (index == ARRAY_SIZE(sprd_port))
>> + return -EBUSY;
>> +
>> + index = sprd_probe_dt_alias(index, &pdev->dev);
>> +
>> + sprd_port[index] = devm_kzalloc(&pdev->dev,
>> + sizeof(*sprd_port[index]), GFP_KERNEL);
>> + if (!sprd_port[index])
>> + return -ENOMEM;
>> +
>> + up = &sprd_port[index]->port;
>> + up->dev = &pdev->dev;
>> + up->line = index;
>> + up->type = PORT_SPRD;
>> + up->iotype = SERIAL_IO_PORT;
>> + up->uartclk = SPRD_DEF_RATE;
>> + up->fifosize = SPRD_FIFO_SIZE;
>> + up->ops = &serial_sprd_ops;
>> + up->flags = UPF_BOOT_AUTOCONF;
>> +
>> + clk = devm_clk_get(&pdev->dev, NULL);
>> + if (!IS_ERR(clk))
>> + up->uartclk = clk_get_rate(clk);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev, "not provide mem resource\n");
>> + return -ENODEV;
>> + }
>
> This check is not required. It will be done by devm_ioremap_resource()
>
>> + up->mapbase = res->start;
Accessing of 'res' has to be done after devm_ioremap_resource()
>> + up->membase = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(up->membase))
>> + return PTR_ERR(up->membase);
>> +
>>
>
--
Thanks,
Varka Bhadram.
^ permalink raw reply
* Re: [PATCH v10 2/2] tty/serial: Add Spreadtrum sc9836-uart driver support
From: Varka Bhadram @ 2015-01-29 15:26 UTC (permalink / raw)
To: Chunyan Zhang, gregkh
Cc: robh+dt, mark.rutland, arnd, gnomes, peter, pawel.moll,
ijc+devicetree, galak, grant.likely, jslaby, heiko, jason,
florian.vaussard, andrew, hytszk, antonynpavlov, shawn.guo,
orsonzhai, geng.ren, zhizhou.zhang, lanqing.liu, zhang.lyra,
wei.qiao, devicetree, linux-kernel, linux-serial, linux-api,
linux-arm-kernel
In-Reply-To: <1422443324-25082-3-git-send-email-chunyan.zhang@spreadtrum.com>
Hi,
On Wednesday 28 January 2015 04:38 PM, Chunyan Zhang wrote:
> Add a full sc9836-uart driver for SC9836 SoC which is based on the
> spreadtrum sharkl64 platform.
> This driver also support earlycon.
>
> Originally-by: Lanqing Liu <lanqing.liu@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> Signed-off-by: Chunyan Zhang <chunyan.zhang@spreadtrum.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Peter Hurley <peter@hurleysoftware.com>
> ---
> drivers/tty/serial/Kconfig | 18 +
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/sprd_serial.c | 793 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/serial_core.h | 3 +
> 4 files changed, 815 insertions(+)
> create mode 100644 drivers/tty/serial/sprd_serial.c
>
(...)
> +static int sprd_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct uart_port *up;
> + struct clk *clk;
> + int irq;
> + int index;
> + int ret;
> +
> + for (index = 0; index < ARRAY_SIZE(sprd_port); index++)
> + if (sprd_port[index] == NULL)
> + break;
> +
> + if (index == ARRAY_SIZE(sprd_port))
> + return -EBUSY;
> +
> + index = sprd_probe_dt_alias(index, &pdev->dev);
> +
> + sprd_port[index] = devm_kzalloc(&pdev->dev,
> + sizeof(*sprd_port[index]), GFP_KERNEL);
> + if (!sprd_port[index])
> + return -ENOMEM;
> +
> + up = &sprd_port[index]->port;
> + up->dev = &pdev->dev;
> + up->line = index;
> + up->type = PORT_SPRD;
> + up->iotype = SERIAL_IO_PORT;
> + up->uartclk = SPRD_DEF_RATE;
> + up->fifosize = SPRD_FIFO_SIZE;
> + up->ops = &serial_sprd_ops;
> + up->flags = UPF_BOOT_AUTOCONF;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (!IS_ERR(clk))
> + up->uartclk = clk_get_rate(clk);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "not provide mem resource\n");
> + return -ENODEV;
> + }
This check is not required. It will be done by devm_ioremap_resource()
> + up->mapbase = res->start;
> + up->membase = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(up->membase))
> + return PTR_ERR(up->membase);
> +
>
--
Thanks,
Varka Bhadram.
^ permalink raw reply
* Re: [PATCH v2] samsung-laptop: enable better lid handling
From: Julijonas Kikutis @ 2015-01-29 13:12 UTC (permalink / raw)
To: Darren Hart
Cc: Corentin Chary, open list:ABI/API, open list,
open list:SAMSUNG LAPTOP DR...
In-Reply-To: <20150129052021.GA115032@vmdeb7>
> Patch is generally fine, thanks for addressing my comments. Prior to merging I
> always run checkpatch.pl just in case I missed anything obvious:
>
> $ scripts/checkpatch.pl ~/samsung/01-lid-handling.patch
> WARNING: Prefer kstrto<type> to single variable sscanf
> #219: FILE: drivers/platform/x86/samsung-laptop.c:900:
> + if (!count || sscanf(buf, "%i", &value) != 1)
> + return -EINVAL;
>
> total: 0 errors, 1 warnings, 219 lines checked
>
> Please always run checkpatch.pl. It isn't sufficient and doesn't catch
> everything, but it is a minimum bar kind of thing.
I did run checkpatch.pl but left sscanf to be consistent with
set_battery_life_extender and set_usb_charge functions. Nevertheless, I
have just sent the third version of the patch with sscanf changed to
kstrtoint.
Thank you.
Julijonas
^ permalink raw reply
* [PATCH v3] samsung-laptop: enable better lid handling
From: Julijonas Kikutis @ 2015-01-29 13:04 UTC (permalink / raw)
To: Darren Hart
Cc: Julijonas Kikutis, Corentin Chary, open list:ABI/API, open list,
open list:SAMSUNG LAPTOP DR...
In-Reply-To: <1418332715-10547-1-git-send-email-julijonas.kikutis@gmail.com>
Some Samsung laptops with SABI3 delay the sleep for 10 seconds after
the lid is closed and do not wake up from sleep after the lid is opened.
A SABI command is needed to enable the better behavior.
Command = 0x6e, d0 = 0x81 enables this behavior. Returns d0 = 0x01.
Command = 0x6e, d0 = 0x80 disables this behavior. Returns d0 = 0x00.
Command = 0x6d and any d0 queries the state. This returns:
d0 = 0x00000*01, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is enabled.
d0 = 0x00000*00, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is disabled.
Where * is 0 - laptop has never slept or hibernated after switch on,
1 - laptop has hibernated just before,
2 - laptop has slept just before.
Patch addresses bug https://bugzilla.kernel.org/show_bug.cgi?id=75901 .
It adds a sysfs attribute lid_handling with a description and also an
addition to the quirks structure to enable the mode by default.
A user with another laptop in the bug report says that "power button has
to be pressed twice to wake the machine" when he or she enabled the mode
manually using the SABI command. Therefore, it is enabled by default
only for the single laptop that I have tested.
Signed-off-by: Julijonas Kikutis <julijonas.kikutis@gmail.com>
---
.../ABI/testing/sysfs-driver-samsung-laptop | 8 ++
drivers/platform/x86/samsung-laptop.c | 120 ++++++++++++++++++++-
2 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-samsung-laptop b/Documentation/ABI/testing/sysfs-driver-samsung-laptop
index 678819a..63c1ad0 100644
--- a/Documentation/ABI/testing/sysfs-driver-samsung-laptop
+++ b/Documentation/ABI/testing/sysfs-driver-samsung-laptop
@@ -35,3 +35,11 @@ Contact: Corentin Chary <corentin.chary@gmail.com>
Description: Use your USB ports to charge devices, even
when your laptop is powered off.
1 means enabled, 0 means disabled.
+
+What: /sys/devices/platform/samsung/lid_handling
+Date: December 11, 2014
+KernelVersion: 3.19
+Contact: Julijonas Kikutis <julijonas.kikutis@gmail.com>
+Description: Some Samsung laptops handle lid closing quicker and
+ only handle lid opening with this mode enabled.
+ 1 means enabled, 0 means disabled.
diff --git a/drivers/platform/x86/samsung-laptop.c b/drivers/platform/x86/samsung-laptop.c
index ff765d8..83ec424 100644
--- a/drivers/platform/x86/samsung-laptop.c
+++ b/drivers/platform/x86/samsung-laptop.c
@@ -124,6 +124,10 @@ struct sabi_commands {
u16 get_wireless_status;
u16 set_wireless_status;
+ /* 0x80 is off, 0x81 is on */
+ u16 get_lid_handling;
+ u16 set_lid_handling;
+
/* 0x81 to read, (0x82 | level << 8) to set, 0xaabb to enable */
u16 kbd_backlight;
@@ -194,6 +198,9 @@ static const struct sabi_config sabi_configs[] = {
.get_wireless_status = 0xFFFF,
.set_wireless_status = 0xFFFF,
+ .get_lid_handling = 0xFFFF,
+ .set_lid_handling = 0xFFFF,
+
.kbd_backlight = 0xFFFF,
.set_linux = 0x0a,
@@ -254,6 +261,9 @@ static const struct sabi_config sabi_configs[] = {
.get_wireless_status = 0x69,
.set_wireless_status = 0x6a,
+ .get_lid_handling = 0x6d,
+ .set_lid_handling = 0x6e,
+
.kbd_backlight = 0x78,
.set_linux = 0xff,
@@ -353,6 +363,7 @@ struct samsung_quirks {
bool broken_acpi_video;
bool four_kbd_backlight_levels;
bool enable_kbd_backlight;
+ bool lid_handling;
};
static struct samsung_quirks samsung_unknown = {};
@@ -366,6 +377,10 @@ static struct samsung_quirks samsung_np740u3e = {
.enable_kbd_backlight = true,
};
+static struct samsung_quirks samsung_lid_handling = {
+ .lid_handling = true,
+};
+
static bool force;
module_param(force, bool, 0);
MODULE_PARM_DESC(force,
@@ -830,10 +845,76 @@ static ssize_t set_usb_charge(struct device *dev,
static DEVICE_ATTR(usb_charge, S_IWUSR | S_IRUGO,
get_usb_charge, set_usb_charge);
+static int read_lid_handling(struct samsung_laptop *samsung)
+{
+ const struct sabi_commands *commands = &samsung->config->commands;
+ struct sabi_data data;
+ int retval;
+
+ if (commands->get_lid_handling == 0xFFFF)
+ return -ENODEV;
+
+ memset(&data, 0, sizeof(data));
+ retval = sabi_command(samsung, commands->get_lid_handling,
+ &data, &data);
+
+ if (retval)
+ return retval;
+
+ return data.data[0] & 0x1;
+}
+
+static int write_lid_handling(struct samsung_laptop *samsung,
+ int enabled)
+{
+ const struct sabi_commands *commands = &samsung->config->commands;
+ struct sabi_data data;
+
+ memset(&data, 0, sizeof(data));
+ data.data[0] = 0x80 | enabled;
+ return sabi_command(samsung, commands->set_lid_handling,
+ &data, NULL);
+}
+
+static ssize_t get_lid_handling(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct samsung_laptop *samsung = dev_get_drvdata(dev);
+ int ret;
+
+ ret = read_lid_handling(samsung);
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t set_lid_handling(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct samsung_laptop *samsung = dev_get_drvdata(dev);
+ int ret, value;
+
+ if (!count || kstrtoint(buf, 0, &value) != 0)
+ return -EINVAL;
+
+ ret = write_lid_handling(samsung, !!value);
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static DEVICE_ATTR(lid_handling, S_IWUSR | S_IRUGO,
+ get_lid_handling, set_lid_handling);
+
static struct attribute *platform_attributes[] = {
&dev_attr_performance_level.attr,
&dev_attr_battery_life_extender.attr,
&dev_attr_usb_charge.attr,
+ &dev_attr_lid_handling.attr,
NULL
};
@@ -956,6 +1037,22 @@ static int __init samsung_rfkill_init(struct samsung_laptop *samsung)
return 0;
}
+static void samsung_lid_handling_exit(struct samsung_laptop *samsung)
+{
+ if (samsung->quirks->lid_handling)
+ write_lid_handling(samsung, 0);
+}
+
+static int __init samsung_lid_handling_init(struct samsung_laptop *samsung)
+{
+ int retval = 0;
+
+ if (samsung->quirks->lid_handling)
+ retval = write_lid_handling(samsung, 1);
+
+ return retval;
+}
+
static int kbd_backlight_enable(struct samsung_laptop *samsung)
{
const struct sabi_commands *commands = &samsung->config->commands;
@@ -1111,7 +1208,7 @@ static int __init samsung_backlight_init(struct samsung_laptop *samsung)
}
static umode_t samsung_sysfs_is_visible(struct kobject *kobj,
- struct attribute *attr, int idx)
+ struct attribute *attr, int idx)
{
struct device *dev = container_of(kobj, struct device, kobj);
struct platform_device *pdev = to_platform_device(dev);
@@ -1124,6 +1221,8 @@ static umode_t samsung_sysfs_is_visible(struct kobject *kobj,
ok = !!(read_battery_life_extender(samsung) >= 0);
if (attr == &dev_attr_usb_charge.attr)
ok = !!(read_usb_charge(samsung) >= 0);
+ if (attr == &dev_attr_lid_handling.attr)
+ ok = !!(read_lid_handling(samsung) >= 0);
return ok ? attr->mode : 0;
}
@@ -1436,6 +1535,9 @@ static int samsung_pm_notification(struct notifier_block *nb,
samsung->quirks->enable_kbd_backlight)
kbd_backlight_enable(samsung);
+ if (val == PM_POST_HIBERNATION && samsung->quirks->lid_handling)
+ write_lid_handling(samsung, 1);
+
return 0;
}
@@ -1578,6 +1680,15 @@ static struct dmi_system_id __initdata samsung_dmi_table[] = {
},
.driver_data = &samsung_np740u3e,
},
+ {
+ .callback = samsung_dmi_matched,
+ .ident = "300V3Z/300V4Z/300V5Z",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "SAMSUNG ELECTRONICS CO., LTD."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "300V3Z/300V4Z/300V5Z"),
+ },
+ .driver_data = &samsung_lid_handling,
+ },
{ },
};
MODULE_DEVICE_TABLE(dmi, samsung_dmi_table);
@@ -1648,6 +1759,10 @@ static int __init samsung_init(void)
if (ret)
goto error_leds;
+ ret = samsung_lid_handling_init(samsung);
+ if (ret)
+ goto error_lid_handling;
+
ret = samsung_debugfs_init(samsung);
if (ret)
goto error_debugfs;
@@ -1659,6 +1774,8 @@ static int __init samsung_init(void)
return ret;
error_debugfs:
+ samsung_lid_handling_exit(samsung);
+error_lid_handling:
samsung_leds_exit(samsung);
error_leds:
samsung_rfkill_exit(samsung);
@@ -1683,6 +1800,7 @@ static void __exit samsung_exit(void)
unregister_pm_notifier(&samsung->pm_nb);
samsung_debugfs_exit(samsung);
+ samsung_lid_handling_exit(samsung);
samsung_leds_exit(samsung);
samsung_rfkill_exit(samsung);
samsung_backlight_exit(samsung);
--
2.2.2
^ permalink raw reply related
* Re: [PATCH v2 linux-trace 1/8] tracing: attach eBPF programs to tracepoints and syscalls
From: Namhyung Kim @ 2015-01-29 12:35 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Steven Rostedt, Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa,
Masami Hiramatsu, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuzSDYYWneoTy9=AmMa9=x4owWbWsXod=Do54+yj4Je3=Q@mail.gmail.com>
On Thu, Jan 29, 2015 at 4:04 PM, Alexei Starovoitov <ast-uqk4Ao+rVK5Wk0Htik3J/w@public.gmane.org> wrote:
> On Wed, Jan 28, 2015 at 10:41 PM, Namhyung Kim <namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>
>> I think it's not a problem of bpf. An user process can be killed
>> anytime while it enabed events without bpf. The only thing it should
>> care is the auto-unload IMHO.
>
> ok. I think it does indeed make sense to decouple the logic.
> We can add 'auto_enable' file to achieve desired Ctrl-C behavior.
> While the 'auto_enable' file is open the event will be enabled
> and writes to 'enable' file will be ignored.
> As soon as file closes, the event is auto-disabled.
> Then user space will use 'bpf' file to attach/auto-unload
> and 'auto_enable' file together.
> Seem there would be a use for such 'auto_enable'
> without bpf as well.
Why do you want such an 'auto_enable' feature? I guess it's enough
just to keep an event in the soft-disabled state and run a bpf program
before the check.
>
>> I'm okay for not calling bpf program in NMI but not for disabling events.
>>
>> Suppose an user was collecting an event (including in NMI) and then
>> [s]he also wanted to run a bpf program. So [s]he wrote a program
>> always return 1. But after attaching the program, it didn't record
>> the event in NMI.. Isn't that a problem?
>
> ok, I think 'if (in_nmi()) return 1;' will work then, right?
> Or you're thinking something else ?
Nope, returning 1 would be okay..
>
>> Right. I think bpf programs belong to a user process but events are
>> global resource. Maybe you also need to consider attaching bpf
>> program via perf (ioctl?) interface..
>
> yes. I did. Please see my reply to Masami.
> ioctl only works for tracepoints.
What was the problem of kprobes then? :)
Thanks,
Namhyung
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Andy Lutomirski @ 2015-01-29 12:09 UTC (permalink / raw)
To: Daniel Mack
Cc: Arnd Bergmann, Ted Ts'o, Linux API, Michael Kerrisk,
One Thousand Gnomes, Austin S Hemmelgarn, Tom Gundersen,
Greg Kroah-Hartman, linux-kernel, David Herrmann,
Eric W. Biederman, Djalal Harouni, Johannes Stezenbach,
Christoph Hellwig
In-Reply-To: <54CA1CA2.9060005@zonque.org>
On Jan 29, 2015 6:42 AM, "Daniel Mack" <daniel@zonque.org> wrote:
>
> On 01/29/2015 12:25 PM, Andy Lutomirski wrote:
> > On Jan 29, 2015 3:53 AM, "Daniel Mack" <daniel@zonque.org> wrote:
>
> >> Also note that if a receiving peer opts in for a certain piece of
> >> metadata, it should do that that for a good reason, because it needs
> >> that data to process a request. Letting kdbus do the work of providing
> >> such information is still a lot faster than having the receiving peer
> >> gather it itself, as that would involve more syscalls and more context
> >> switches (let alone the fact that doing so is inherently racy, as
> >> explained in earlier threads).
>
> > Given that I see almost no advantage to send-time metadata, and I see
> > three disadvantages (slower, inconsistent with the basic POSIX model,
> > and inconsistent with existing user-space dbus), I still don't see why
> > you designed it this way.
>
> Because effective information about tasks may change over time, and
> D-Bus is a connection-less protocol that has no notion of peer-to-peer
> connections.
>
> As we explained before, currently, D-Bus peers do collect the same
> information already if they need to have them, but they have to do deal
> with the inherit races in such cases. kdbus is closing the gap by
> optionally providing the same information along with each message, if
> requested.
In all these discussions, no one ever gave a decent example use case.
If a process drops some privilege, it must close all fds it has that
captured its old privilege. This has nothing to do with kdbus. With
kdbus, you still need to close and reopen your kdbus fd, unless you've
disabled that bit of metadata, so using send-time metadata hasn't
bought you benefit that I can see.
I agree that the design seems to have improved to a state of being at
least decent, but that doesn't mean that using send-time metadata is a
good idea for systemd or for anything else.
>
> > There's an added disadvantage of the current design: if a kdbus user
> > is communicating with a traditional d-bus user using the proxy, then
> > IIUC the credentials at the time of connection get used.
>
> That's not quite true any more. After our discussion in v2, we agreed on
> dropping this detail. If you're using the proxy, no metadata is attached
> to messages any more. Userspace has to gather this information in the
> traditional, racy way in such cases. You are right - metadata about the
> proxy task is of no interest here, and hence dropping the information
> altogether is the most consistent thing we can do.
>
> But again - that metadata thing just an optional feature. People
> developing with the bare kernel-level API are free to ignore all that
> and just just kdbus as low-level protocol for reliable multicast. Note
> that in such cases, you would still be able to retrieve the connect-time
> metadata if that's needed.
>
It's an optional feature that will get used, non-optionally, thousands
of times on each boot, apparently. Keep in mind that it's also a
scalability problem because it takes locks. If it ever gets used
thousands of times per CPU on a big thousand-core machine, it's going
to suck, and you'll have backed yourself into a corner.
--Andy
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Daniel Mack @ 2015-01-29 11:42 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Michael Kerrisk, Arnd Bergmann, Theodore T'so, Linux API,
One Thousand Gnomes, Austin S Hemmelgarn, Tom Gundersen,
Greg Kroah-Hartman, linux-kernel, Eric W. Biederman,
David Herrmann, Djalal Harouni, Johannes Stezenbach,
Christoph Hellwig
In-Reply-To: <CALCETrV6P+Lx5FRLyDNubG18tqCTLprkXJf=82aa5DsRHjkuKA@mail.gmail.com>
On 01/29/2015 12:25 PM, Andy Lutomirski wrote:
> On Jan 29, 2015 3:53 AM, "Daniel Mack" <daniel@zonque.org> wrote:
>> Also note that if a receiving peer opts in for a certain piece of
>> metadata, it should do that that for a good reason, because it needs
>> that data to process a request. Letting kdbus do the work of providing
>> such information is still a lot faster than having the receiving peer
>> gather it itself, as that would involve more syscalls and more context
>> switches (let alone the fact that doing so is inherently racy, as
>> explained in earlier threads).
> Given that I see almost no advantage to send-time metadata, and I see
> three disadvantages (slower, inconsistent with the basic POSIX model,
> and inconsistent with existing user-space dbus), I still don't see why
> you designed it this way.
Because effective information about tasks may change over time, and
D-Bus is a connection-less protocol that has no notion of peer-to-peer
connections.
As we explained before, currently, D-Bus peers do collect the same
information already if they need to have them, but they have to do deal
with the inherit races in such cases. kdbus is closing the gap by
optionally providing the same information along with each message, if
requested.
> There's an added disadvantage of the current design: if a kdbus user
> is communicating with a traditional d-bus user using the proxy, then
> IIUC the credentials at the time of connection get used.
That's not quite true any more. After our discussion in v2, we agreed on
dropping this detail. If you're using the proxy, no metadata is attached
to messages any more. Userspace has to gather this information in the
traditional, racy way in such cases. You are right - metadata about the
proxy task is of no interest here, and hence dropping the information
altogether is the most consistent thing we can do.
But again - that metadata thing just an optional feature. People
developing with the bare kernel-level API are free to ignore all that
and just just kdbus as low-level protocol for reliable multicast. Note
that in such cases, you would still be able to retrieve the connect-time
metadata if that's needed.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Andy Lutomirski @ 2015-01-29 11:25 UTC (permalink / raw)
To: Daniel Mack
Cc: Michael Kerrisk, Arnd Bergmann, Theodore T'so, Linux API,
One Thousand Gnomes, Austin S Hemmelgarn, Tom Gundersen,
Greg Kroah-Hartman, linux-kernel, Eric W. Biederman,
David Herrmann, Djalal Harouni, Johannes Stezenbach,
Christoph Hellwig
In-Reply-To: <54C9F525.4040703@zonque.org>
On Jan 29, 2015 3:53 AM, "Daniel Mack" <daniel@zonque.org> wrote:
>
> Hi Andy,
>
> On 01/27/2015 05:03 PM, Andy Lutomirski wrote:
> > On Tue, Jan 27, 2015 at 7:05 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>
> >> A 16byte copy does not affect the performance of kdbus message
> >> transactions in any way that matters.
>
> > What are the performance goals of kdbus? How fast is it ever intended
> > to be?
>
> One of the design goals of kdbus is to speed up a typical D-Bus message
> turnaround. That is, to minimize the number of context switches it
> currently takes to get a message across the bus, and to avoid
> unnecessary extra payload copies.
>
> Even though I'm sure there's still room for future improvement, the
> benchmark test we provided in the kernel self-tests shows that basic
> data transmission performance is roughly comparable to that of UDS for
> smaller payloads. For payloads of bigger sizes (>128kb), kdbus is
> actually faster due to its zero-copy mechanism.
>
> > The reason I ask is that, in the current design, kdbus
> > collects "metadata" (credentials and other identifying information,
> > collected in kdbus_meta_proc_collect) from the sender of every message
> > *at send time*. [1] This is slow, and it will always be slow. The
> > slowness of this operation will, in my personal system performance
> > crystal ball, overshadow the cost of a 16 byte copy by several orders
> > of magnitude.
>
> That's certainly true, but that's not a contradiction to the performance
> argument. Please keep in mind that if a receiving peer does not request
> any metadata, the kernel doesn't collect and attach any. We know that
> gathering of some of the metadata comes at a price, which is why we
> split up the information is such fine-grained pieces.
>
> Also note that if a receiving peer opts in for a certain piece of
> metadata, it should do that that for a good reason, because it needs
> that data to process a request. Letting kdbus do the work of providing
> such information is still a lot faster than having the receiving peer
> gather it itself, as that would involve more syscalls and more context
> switches (let alone the fact that doing so is inherently racy, as
> explained in earlier threads).
All this is true, but if you used connect-time metadata, this would be
a non-issue.
Given that I see almost no advantage to send-time metadata, and I see
three disadvantages (slower, inconsistent with the basic POSIX model,
and inconsistent with existing user-space dbus), I still don't see why
you designed it this way.
There's an added disadvantage of the current design: if a kdbus user
is communicating with a traditional d-bus user using the proxy, then
IIUC the credentials at the time of connection get used.
In summary, the current design is (a) unlike almost everything else
that uses file descriptors, (b) much slower, (c) different from
traditional d-bus, and (d) gives inconsistent behavior to new clients
depending on what server they're connecting to.
--Andy
>
> So, yes, collecting metadata can slow down message exchange, but after
> all, that's an optional feature that has to be used with sense. I'll add
> some words on that to the man-pages.
>
>
> HTH,
> Daniel
>
^ permalink raw reply
* Re: [RESEND] [PATCH] block: create ioctl to discard-or-zeroout a range of blocks
From: Arnd Bergmann @ 2015-01-29 10:02 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Jens Axboe, Christoph Hellwig, linux-kernel, linux-fsdevel,
linux-api, Jeff Layton, J. Bruce Fields
In-Reply-To: <20150129020025.GE9981@birch.djwong.org>
On Wednesday 28 January 2015 18:00:25 Darrick J. Wong wrote:
> Create a new ioctl to expose the block layer's newfound ability to
> issue either a zeroing discard, a WRITE SAME with a zero page, or a
> regular write with the zero page. This BLKZEROOUT2 ioctl takes
> {start, length, flags} as parameters. So far, the only flag available
> is to enable the zeroing discard part -- without it, the call invokes
> the old BLKZEROOUT behavior. start and length have the same meaning
> as in BLKZEROOUT.
>
> Furthermore, because BLKZEROOUT2 issues commands directly to the
> storage device, we must invalidate the page cache (as a regular
> O_DIRECT write would do) to avoid returning stale cache contents at a
> later time.
>
> This patch depends on "block: Add discard flag to
> blkdev_issue_zeroout() function" in Jens' for-3.20/core branch.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>
Would this work ok for devices that fill discarded areas with all-ones
instead of all-zeroes? I believe SD cards can do either.
Arnd
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Daniel Mack @ 2015-01-29 8:53 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Herrmann, Michael Kerrisk (man-pages), Greg Kroah-Hartman,
Austin S Hemmelgarn, Arnd Bergmann, Eric W. Biederman,
One Thousand Gnomes, Tom Gundersen, Theodore T'so, Linux API,
linux-kernel, Djalal Harouni, Johannes Stezenbach,
Christoph Hellwig
In-Reply-To: <CALCETrU1O6LjhrXSH2aXWFt2WGh=ssrq4K9HD10f_Z55iT=Otw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Andy,
On 01/27/2015 05:03 PM, Andy Lutomirski wrote:
> On Tue, Jan 27, 2015 at 7:05 AM, David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> A 16byte copy does not affect the performance of kdbus message
>> transactions in any way that matters.
> What are the performance goals of kdbus? How fast is it ever intended
> to be?
One of the design goals of kdbus is to speed up a typical D-Bus message
turnaround. That is, to minimize the number of context switches it
currently takes to get a message across the bus, and to avoid
unnecessary extra payload copies.
Even though I'm sure there's still room for future improvement, the
benchmark test we provided in the kernel self-tests shows that basic
data transmission performance is roughly comparable to that of UDS for
smaller payloads. For payloads of bigger sizes (>128kb), kdbus is
actually faster due to its zero-copy mechanism.
> The reason I ask is that, in the current design, kdbus
> collects "metadata" (credentials and other identifying information,
> collected in kdbus_meta_proc_collect) from the sender of every message
> *at send time*. [1] This is slow, and it will always be slow. The
> slowness of this operation will, in my personal system performance
> crystal ball, overshadow the cost of a 16 byte copy by several orders
> of magnitude.
That's certainly true, but that's not a contradiction to the performance
argument. Please keep in mind that if a receiving peer does not request
any metadata, the kernel doesn't collect and attach any. We know that
gathering of some of the metadata comes at a price, which is why we
split up the information is such fine-grained pieces.
Also note that if a receiving peer opts in for a certain piece of
metadata, it should do that that for a good reason, because it needs
that data to process a request. Letting kdbus do the work of providing
such information is still a lot faster than having the receiving peer
gather it itself, as that would involve more syscalls and more context
switches (let alone the fact that doing so is inherently racy, as
explained in earlier threads).
So, yes, collecting metadata can slow down message exchange, but after
all, that's an optional feature that has to be used with sense. I'll add
some words on that to the man-pages.
HTH,
Daniel
^ permalink raw reply
* Re: [PATCH v2 linux-trace 1/8] tracing: attach eBPF programs to tracepoints and syscalls
From: Alexei Starovoitov @ 2015-01-29 7:04 UTC (permalink / raw)
To: Namhyung Kim
Cc: Steven Rostedt, Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa,
Masami Hiramatsu, Linux API, Network Development, LKML
On Wed, Jan 28, 2015 at 10:41 PM, Namhyung Kim <namhyung@kernel.org> wrote:
>
> I think it's not a problem of bpf. An user process can be killed
> anytime while it enabed events without bpf. The only thing it should
> care is the auto-unload IMHO.
ok. I think it does indeed make sense to decouple the logic.
We can add 'auto_enable' file to achieve desired Ctrl-C behavior.
While the 'auto_enable' file is open the event will be enabled
and writes to 'enable' file will be ignored.
As soon as file closes, the event is auto-disabled.
Then user space will use 'bpf' file to attach/auto-unload
and 'auto_enable' file together.
Seem there would be a use for such 'auto_enable'
without bpf as well.
> I'm okay for not calling bpf program in NMI but not for disabling events.
>
> Suppose an user was collecting an event (including in NMI) and then
> [s]he also wanted to run a bpf program. So [s]he wrote a program
> always return 1. But after attaching the program, it didn't record
> the event in NMI.. Isn't that a problem?
ok, I think 'if (in_nmi()) return 1;' will work then, right?
Or you're thinking something else ?
> Right. I think bpf programs belong to a user process but events are
> global resource. Maybe you also need to consider attaching bpf
> program via perf (ioctl?) interface..
yes. I did. Please see my reply to Masami.
ioctl only works for tracepoints.
^ permalink raw reply
* Re: [PATCH v2 linux-trace 1/8] tracing: attach eBPF programs to tracepoints and syscalls
From: Namhyung Kim @ 2015-01-29 6:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Steven Rostedt, Ingo Molnar, Arnaldo Carvalho de Melo, Jiri Olsa,
Masami Hiramatsu, Linux API, Network Development, LKML
In-Reply-To: <CAMEtUuz+N52ms4ZFh8+fSGhg1UaXpBdk9RVPU_zBDCjaZ4bj4A@mail.gmail.com>
On Thu, Jan 29, 2015 at 1:40 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On Wed, Jan 28, 2015 at 4:46 PM, Namhyung Kim <namhyung@kernel.org> wrote:
>>>
>>> +static int event_filter_release(struct inode *inode, struct file *filp)
>>> +{
>>> + struct ftrace_event_file *file;
>>> + char buf[2] = "0";
>>> +
>>> + mutex_lock(&event_mutex);
>>> + file = event_file_data(filp);
>>> + if (file) {
>>> + if (file->flags & TRACE_EVENT_FL_BPF) {
>>> + /* auto-disable the filter */
>>> + ftrace_event_enable_disable(file, 0);
>>
>> Hmm.. what if user already enabled an event, attached a bpf filter and
>> then detached the filter - I'm not sure we can always auto-disable
>> it..
>
> why not?
> I think it makes sense auto enable/disable, since that
> is cleaner user experience.
> Otherwise Ctrl-C of the user process will have bpf program dangling.
> not good. If we auto-unload bpf program only, it's equally bad.
> Since Ctrl-C of the process will auto-onload only
> and will keep tracepoint enabled which will be spamming
> the trace buffer.
I think it's not a problem of bpf. An user process can be killed
anytime while it enabed events without bpf. The only thing it should
care is the auto-unload IMHO.
>
>>> +unsigned int trace_filter_call_bpf(struct event_filter *filter, void *ctx)
>>> +{
>>> + unsigned int ret;
>>> +
>>> + if (in_nmi()) /* not supported yet */
>>> + return 0;
>>
>> But doesn't this mean to auto-disable all attached events during NMI
>> as returning 0 will prevent the event going to ring buffer?
>
> well, it means that if tracepoint fired during nmi the program
> won't be called and event won't be sent to trace buffer.
> The program might be broken (like divide by zero) and
> it will self-terminate with 'return 0'
> so zero should be the safest return value that
> causes minimum disturbance to the whole system overall.
I'm okay for not calling bpf program in NMI but not for disabling events.
Suppose an user was collecting an event (including in NMI) and then
[s]he also wanted to run a bpf program. So [s]he wrote a program
always return 1. But after attaching the program, it didn't record
the event in NMI.. Isn't that a problem?
>
>> I think it'd be better to keep an attached event in a soft-disabled
>> state like event trigger and give control of enabling to users..
>
> I think it suffers from the same Ctrl-C issue.
> Say, attaching bpf program activates tracepoint and keeps
> it in soft-disabled. Then user space clears soft-disabled.
> Then user Ctrl-C it. Now bpf program must auto-detach
> and unload, since prog_fd is closing.
> If we don't completely deactivate tracepoint, then
> Ctrl-C will leave the state of the system in the state
> different from it was before user process started running.
> I think we must avoid such situation.
> 'kill pid' should be completely cleaning all resources
> that user process was using.
> Yes. It's different from typical usage of /sys/.../tracing
> that has all global knobs, but, imo, it's cleaner this way.
Right. I think bpf programs belong to a user process but events are
global resource. Maybe you also need to consider attaching bpf
program via perf (ioctl?) interface..
Thanks,
Namhyung
^ permalink raw reply
* Re: [PATCH v2 linux-trace 1/8] tracing: attach eBPF programs to tracepoints and syscalls
From: Alexei Starovoitov @ 2015-01-29 6:39 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Namhyung Kim, Steven Rostedt, Ingo Molnar,
Arnaldo Carvalho de Melo, Jiri Olsa, Linux API,
Network Development, LKML
On Wed, Jan 28, 2015 at 9:39 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
>
> Maybe, would we need a reference counter for each event? :)
when we would want multiple users to attach different programs
to the same event, then yes.
Right now I'd rather have things simple.
> Actually, ftrace event is not similar to perf-event which ktap
> is based on, ftrace event interface is always exported via
> debugfs, this means users can share the event for different
> usage.
yes.I've been thinking to extend perf_event ioctl to attach programs,
but right now it's only supporting tracepoints and kprobe
seems not trivial to add.
So I went for tracefs style of attaching for now.
> One possible other solution is to add a instance-lock interface
> for each ftrace instance and lock it by bpf. Then, other users
> can not enable/disable the events in the instance.
the user space can synchronize itself via flock. kernel doesn't
need to arbitrate. If one user process attached a program
that auto-enabled an event and another process did
'echo 0 > enable', it's fine. I think it's a feature instead of a bug.
Both users are root anyway.
The more we talk about it, the more I like a new 'bpf' file
approach within tracefs (that I've mentioned in cover letter)
with auto-enable/disable logic to make it clear that
it's different from traditional global 'filter' file.
^ permalink raw reply
* [PATCH v2] tpm: fix suspend/resume paths for TPM 2.0
From: Jarkko Sakkinen @ 2015-01-29 5:43 UTC (permalink / raw)
To: Peter Huewe, Ashley Lai, Marcel Selhorst
Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, josh-iaAMLnmF4UmaiuxdJuQwMA,
christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w,
jason.gunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/,
stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
linux-api-u79uwXL29TY76Z2rM5mHXA,
trousers-tech-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
jmorris-gx6/JNMH7DfYtjvyW6yDsg, Jarkko Sakkinen
Fixed suspend/resume paths for TPM 2.0 and consolidated all the
associated code to the tpm_pm_suspend() and tpm_pm_resume()
functions. Resume path should be handled by the firmware, i.e.
Startup(CLEAR) for hibernate and Startup(STATE) for suspend.
There might be some non-PC embedded devices in the future where
Startup() is not the handled by the FW but fixing the code for
those IMHO should be postponed until there is hardware available
to test the fixes although extra Startup in the driver code is
essentially a NOP.
Added Shutdown(CLEAR) to the remove paths of TIS and CRB drivers.
Changed tpm2_shutdown() to a void function because there isn't
much you can do except print an error message if this fails with
a system error.
Reported-by: Peter Hüwe <PeterHuewe-Mmb7MZpHnFY@public.gmane.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
drivers/char/tpm/tpm-interface.c | 6 ++++--
drivers/char/tpm/tpm.h | 2 +-
drivers/char/tpm/tpm2-cmd.c | 19 +++++++++++--------
drivers/char/tpm/tpm_crb.c | 20 +++++---------------
drivers/char/tpm/tpm_tis.c | 26 +++++++++++++-------------
5 files changed, 34 insertions(+), 39 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index bf53a37..e85d341 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -901,8 +901,10 @@ int tpm_pm_suspend(struct device *dev)
if (chip == NULL)
return -ENODEV;
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- return tpm2_shutdown(chip, TPM2_SU_CLEAR);
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ tpm2_shutdown(chip, TPM2_SU_STATE);
+ return 0;
+ }
/* for buggy tpm, flush pcrs with extend to selected dummy */
if (tpm_suspend_pcr) {
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7b0727c..a2ce379 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -432,7 +432,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
extern int tpm2_startup(struct tpm_chip *chip, u16 startup_type);
-extern int tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
+extern void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
extern unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *, u32);
extern int tpm2_do_selftest(struct tpm_chip *chip);
extern int tpm2_gen_interrupt(struct tpm_chip *chip, bool quiet);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 1abe650..f2f38a5 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -456,20 +456,23 @@ static const struct tpm_input_header tpm2_shutdown_header = {
* @chip: TPM chip to use.
* @shutdown_type shutdown type. The value is either
* TPM_SU_CLEAR or TPM_SU_STATE.
- *
- * 0 is returned when the operation is successful. If a negative number is
- * returned it remarks a POSIX error code. If a positive number is returned
- * it remarks a TPM error.
*/
-int tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
+void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
{
struct tpm2_cmd cmd;
+ int rc;
cmd.header.in = tpm2_shutdown_header;
-
cmd.params.startup_in.startup_type = cpu_to_be16(shutdown_type);
- return tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
- "stopping the TPM");
+
+ rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), "stopping the TPM");
+
+ /* In places where shutdown command is sent there's no much we can do
+ * except print the error code on a system failure.
+ */
+ if (rc < 0)
+ dev_warn(chip->pdev, "transmit returned %d while stopping the TPM",
+ rc);
}
EXPORT_SYMBOL_GPL(tpm2_shutdown);
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 3dd23cf..b26ceee 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -95,21 +95,7 @@ struct crb_priv {
u8 __iomem *rsp;
};
-#ifdef CONFIG_PM_SLEEP
-static int crb_resume(struct device *dev)
-{
- int rc;
- struct tpm_chip *chip = dev_get_drvdata(dev);
-
- rc = tpm2_shutdown(chip, TPM2_SU_STATE);
- if (!rc)
- rc = tpm2_do_selftest(chip);
-
- return rc;
-}
-#endif
-
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, crb_resume);
+static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
static u8 crb_status(struct tpm_chip *chip)
{
@@ -326,6 +312,10 @@ static int crb_acpi_remove(struct acpi_device *device)
struct tpm_chip *chip = dev_get_drvdata(dev);
tpm_chip_unregister(chip);
+
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ tpm2_shutdown(chip, TPM2_SU_CLEAR);
+
return 0;
}
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 6725bef..e12b3ab 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -588,6 +588,9 @@ MODULE_PARM_DESC(interrupts, "Enable interrupts");
static void tpm_tis_remove(struct tpm_chip *chip)
{
+ if (chip->flags & TPM_CHIP_FLAG_TPM2)
+ tpm2_shutdown(chip, TPM2_SU_CLEAR);
+
iowrite32(~TPM_GLOBAL_INT_ENABLE &
ioread32(chip->vendor.iobase +
TPM_INT_ENABLE(chip->vendor.
@@ -865,25 +868,22 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
static int tpm_tis_resume(struct device *dev)
{
struct tpm_chip *chip = dev_get_drvdata(dev);
- int ret = 0;
+ int ret;
if (chip->vendor.irq)
tpm_tis_reenable_interrupts(chip);
- if (chip->flags & TPM_CHIP_FLAG_TPM2) {
- /* NOP if firmware properly does this. */
- tpm2_startup(chip, TPM2_SU_STATE);
+ ret = tpm_pm_resume(dev);
+ if (ret)
+ return ret;
- ret = tpm2_shutdown(chip, TPM2_SU_STATE);
- if (!ret)
- ret = tpm2_do_selftest(chip);
- } else {
- ret = tpm_pm_resume(dev);
- if (!ret)
- tpm_do_selftest(chip);
- }
+ /* TPM 1.2 requires self-test on resume. This function actually returns
+ * an error code but for unknown reason it isn't handled.
+ */
+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+ tpm_do_selftest(chip);
- return ret;
+ return 0;
}
#endif
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v2 linux-trace 1/8] tracing: attach eBPF programs to tracepoints and syscalls
From: Masami Hiramatsu @ 2015-01-29 5:39 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Namhyung Kim, Steven Rostedt, Ingo Molnar,
Arnaldo Carvalho de Melo, Jiri Olsa, Linux API,
Network Development, LKML
In-Reply-To: <CAMEtUuz+N52ms4ZFh8+fSGhg1UaXpBdk9RVPU_zBDCjaZ4bj4A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
(2015/01/29 13:40), Alexei Starovoitov wrote:
> On Wed, Jan 28, 2015 at 4:46 PM, Namhyung Kim <namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>>
>>> +static int event_filter_release(struct inode *inode, struct file *filp)
>>> +{
>>> + struct ftrace_event_file *file;
>>> + char buf[2] = "0";
>>> +
>>> + mutex_lock(&event_mutex);
>>> + file = event_file_data(filp);
>>> + if (file) {
>>> + if (file->flags & TRACE_EVENT_FL_BPF) {
>>> + /* auto-disable the filter */
>>> + ftrace_event_enable_disable(file, 0);
>>
>> Hmm.. what if user already enabled an event, attached a bpf filter and
>> then detached the filter - I'm not sure we can always auto-disable
>> it..
>
> why not?
> I think it makes sense auto enable/disable, since that
> is cleaner user experience.
Maybe, would we need a reference counter for each event? :)
Actually, ftrace event is not similar to perf-event which ktap
is based on, ftrace event interface is always exported via
debugfs, this means users can share the event for different
usage.
One possible other solution is to add a instance-lock interface
for each ftrace instance and lock it by bpf. Then, other users
can not enable/disable the events in the instance.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt-FCd8Q96Dh0JBDgjK7y7TUQ@public.gmane.org
^ permalink raw reply
* Re: [PATCH v2] samsung-laptop: enable better lid handling
From: Darren Hart @ 2015-01-29 5:20 UTC (permalink / raw)
To: Julijonas Kikutis
Cc: Corentin Chary, open list:ABI/API, open list,
open list:SAMSUNG LAPTOP DR...
In-Reply-To: <1422365168-2428-1-git-send-email-julijonas.kikutis@gmail.com>
On Tue, Jan 27, 2015 at 01:26:06PM +0000, Julijonas Kikutis wrote:
> Some Samsung laptops with SABI3 delay the sleep for 10 seconds after
> the lid is closed and do not wake up from sleep after the lid is opened.
> A SABI command is needed to enable the better behavior.
>
> Command = 0x6e, d0 = 0x81 enables this behavior. Returns d0 = 0x01.
> Command = 0x6e, d0 = 0x80 disables this behavior. Returns d0 = 0x00.
>
> Command = 0x6d and any d0 queries the state. This returns:
> d0 = 0x00000*01, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is enabled.
> d0 = 0x00000*00, d1 = 0x00, d2 = 0x00, d3 = 0x0* when it is disabled.
> Where * is 0 - laptop has never slept or hibernated after switch on,
> 1 - laptop has hibernated just before,
> 2 - laptop has slept just before.
>
> Patch addresses bug https://bugzilla.kernel.org/show_bug.cgi?id=75901 .
> It adds a sysfs attribute lid_handling with a description and also an
> addition to the quirks structure to enable the mode by default.
>
> A user with another laptop in the bug report says that "power button has
> to be pressed twice to wake the machine" when he or she enabled the mode
> manually using the SABI command. Therefore, it is enabled by default
> only for the single laptop that I have tested.
>
> Signed-off-by: Julijonas Kikutis <julijonas.kikutis@gmail.com>
> ---
> .../ABI/testing/sysfs-driver-samsung-laptop | 8 ++
> drivers/platform/x86/samsung-laptop.c | 120 ++++++++++++++++++++-
> 2 files changed, 127 insertions(+), 1 deletion(-)
>
Patch is generally fine, thanks for addressing my comments. Prior to merging I
always run checkpatch.pl just in case I missed anything obvious:
$ scripts/checkpatch.pl ~/samsung/01-lid-handling.patch
WARNING: Prefer kstrto<type> to single variable sscanf
#219: FILE: drivers/platform/x86/samsung-laptop.c:900:
+ if (!count || sscanf(buf, "%i", &value) != 1)
+ return -EINVAL;
total: 0 errors, 1 warnings, 219 lines checked
Please always run checkpatch.pl. It isn't sufficient and doesn't catch
everything, but it is a minimum bar kind of thing.
Thanks,
--
Darren Hart
Intel Open Source Technology Center
^ 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