* Re: [PATCH v11 1/2] crypto: AF_ALG: add AEAD support
From: Stephan Mueller @ 2015-01-26 4:26 UTC (permalink / raw)
To: Herbert Xu
Cc: Daniel Borkmann, 'Quentin Gouchet', 'LKML',
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Neil Horman
In-Reply-To: <20150125235550.GB18212-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Am Montag, 26. Januar 2015, 10:55:50 schrieb Herbert Xu:
Hi Herbert,
> On Wed, Jan 21, 2015 at 02:19:17AM +0100, Stephan Mueller wrote:
> > + /* use the existing memory in an allocated page */
> > + if (ctx->merge) {
> > + sg = sgl->sg + sgl->cur - 1;
> > + len = min_t(unsigned long, len,
> > + PAGE_SIZE - sg->offset - sg->length);
> > + err = memcpy_from_msg(page_address(sg_page(sg)) +
> > + sg->offset + sg->length,
> > + msg, len);
> > + if (err)
> > + goto unlock;
> > +
> > + sg->length += len;
> > + ctx->merge = (sg->offset + sg->length) &
> > + (PAGE_SIZE - 1);
> > +
> > + ctx->used += len;
> > + copied += len;
> > + size -= len;
>
> Need to add a continue here to recheck size != 0.
Why would that be needed?
When size is still != 0 (i.e. the existing buffer is completely filled, we
have still some remaining data), we fall through to the while loop that
generates a new buffer.
If we add a continue here, we start the next iteration in the outer while loop
which again checks for the merging of data in an existing buffer. As this
merging will never happen as we filled that buffer completely in the previous
loop, we always will fall through to the inner while loop. Thus, not having
the check for size != 0 is functional identical to having it (besides, it is
more efficient to not having it).
Note, this case is triggered in my tests, where I use sendmsg with first a
small buffer, followed by a large buffer. And I still can send 65536 bytes to
the kernel.
--
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH v11 1/2] crypto: AF_ALG: add AEAD support
From: Herbert Xu @ 2015-01-26 4:32 UTC (permalink / raw)
To: Stephan Mueller
Cc: Daniel Borkmann, 'Quentin Gouchet', 'LKML',
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Neil Horman
In-Reply-To: <1526868.qaVuSjCOn7-PJstQz4BMNNP20K/wil9xYQuADTiUCJX@public.gmane.org>
On Mon, Jan 26, 2015 at 05:26:33AM +0100, Stephan Mueller wrote:
> Am Montag, 26. Januar 2015, 10:55:50 schrieb Herbert Xu:
>
> Hi Herbert,
>
> > On Wed, Jan 21, 2015 at 02:19:17AM +0100, Stephan Mueller wrote:
> > > + /* use the existing memory in an allocated page */
> > > + if (ctx->merge) {
> > > + sg = sgl->sg + sgl->cur - 1;
> > > + len = min_t(unsigned long, len,
> > > + PAGE_SIZE - sg->offset - sg->length);
> > > + err = memcpy_from_msg(page_address(sg_page(sg)) +
> > > + sg->offset + sg->length,
> > > + msg, len);
> > > + if (err)
> > > + goto unlock;
> > > +
> > > + sg->length += len;
> > > + ctx->merge = (sg->offset + sg->length) &
> > > + (PAGE_SIZE - 1);
> > > +
> > > + ctx->used += len;
> > > + copied += len;
> > > + size -= len;
> >
> > Need to add a continue here to recheck size != 0.
>
> Why would that be needed?
>
> When size is still != 0 (i.e. the existing buffer is completely filled, we
> have still some remaining data), we fall through to the while loop that
> generates a new buffer.
Because when size == 0 you should exit the loop. IOW if the new
data is completely merged you should get out and not continue.
Cheers,
--
Email: Herbert Xu <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v11 1/2] crypto: AF_ALG: add AEAD support
From: Stephan Mueller @ 2015-01-26 4:35 UTC (permalink / raw)
To: Herbert Xu
Cc: Daniel Borkmann, 'Quentin Gouchet', 'LKML',
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Neil Horman
In-Reply-To: <20150126000631.GA18350-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Am Montag, 26. Januar 2015, 11:06:31 schrieb Herbert Xu:
Hi Herbert,
> On Wed, Jan 21, 2015 at 02:19:17AM +0100, Stephan Mueller wrote:
> > + /*
> > + * Require exactly one IOV block as the AEAD operation is a one shot
> > + * due to the authentication tag.
> > + */
> > + if (msg->msg_iter.nr_segs != 1)
> > + return -ENOMSG;
>
> Why does limit exist? The fact that you have to do it in one go does
> not limit the number of receive IOVs, at least not to one.
It seems I have misunderstood you in the last discussion.
in the last discussion [1] I tried to explain why I did not consider multiple
IOVs. In the reply to my answer [2] I seem to have understood that the current
implementation is fine.
So, shall I now implement the multiple IOVs approach outlined in [1]? If yes,
how many IOVs shall I consider?
>
> Cheers,
[1] http://www.spinics.net/lists/linux-crypto/msg12861.html
[2] http://www.spinics.net/lists/linux-crypto/msg12935.html
--
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH v11 1/2] crypto: AF_ALG: add AEAD support
From: Herbert Xu @ 2015-01-26 4:37 UTC (permalink / raw)
To: Stephan Mueller
Cc: Daniel Borkmann, 'Quentin Gouchet', 'LKML',
linux-crypto, linux-api, Neil Horman
In-Reply-To: <13294516.zBBDkdi9pn@tachyon.chronox.de>
On Mon, Jan 26, 2015 at 05:35:07AM +0100, Stephan Mueller wrote:
>
> It seems I have misunderstood you in the last discussion.
I thought you were limiting the receive SGL by ALG_MAX_PAGES rather
than a single IOV entry.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v11 1/2] crypto: AF_ALG: add AEAD support
From: Stephan Mueller @ 2015-01-26 4:39 UTC (permalink / raw)
To: Herbert Xu
Cc: Daniel Borkmann, 'Quentin Gouchet', 'LKML',
linux-crypto, linux-api, Neil Horman
In-Reply-To: <20150126043218.GA20911@gondor.apana.org.au>
Am Montag, 26. Januar 2015, 15:32:18 schrieb Herbert Xu:
Hi Herbert,
> On Mon, Jan 26, 2015 at 05:26:33AM +0100, Stephan Mueller wrote:
> > Am Montag, 26. Januar 2015, 10:55:50 schrieb Herbert Xu:
> >
> > Hi Herbert,
> >
> > > On Wed, Jan 21, 2015 at 02:19:17AM +0100, Stephan Mueller wrote:
> > > > + /* use the existing memory in an allocated page */
> > > > + if (ctx->merge) {
> > > > + sg = sgl->sg + sgl->cur - 1;
> > > > + len = min_t(unsigned long, len,
> > > > + PAGE_SIZE - sg->offset - sg-
>length);
> > > > + err =
memcpy_from_msg(page_address(sg_page(sg)) +
> > > > + sg->offset + sg->length,
> > > > + msg, len);
> > > > + if (err)
> > > > + goto unlock;
> > > > +
> > > > + sg->length += len;
> > > > + ctx->merge = (sg->offset + sg->length) &
> > > > + (PAGE_SIZE - 1);
> > > > +
> > > > + ctx->used += len;
> > > > + copied += len;
> > > > + size -= len;
> > >
> > > Need to add a continue here to recheck size != 0.
> >
> > Why would that be needed?
> >
> > When size is still != 0 (i.e. the existing buffer is completely filled, we
> > have still some remaining data), we fall through to the while loop that
> > generates a new buffer.
>
> Because when size == 0 you should exit the loop. IOW if the new
> data is completely merged you should get out and not continue.
But does it really matter if we consider size == 0 or != at this point? In
case size == 0, the len calculation before the inner while loop will return 0.
Thus the inner while loop will not start. Thus, the current code will not run
the inner loop and exit the outer loop in the next round. So, I am not sure I
see the benefit from another check here.
--
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH v11 1/2] crypto: AF_ALG: add AEAD support
From: Stephan Mueller @ 2015-01-26 4:40 UTC (permalink / raw)
To: Herbert Xu
Cc: Daniel Borkmann, 'Quentin Gouchet', 'LKML',
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Neil Horman
In-Reply-To: <20150126043733.GA20984-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Am Montag, 26. Januar 2015, 15:37:33 schrieb Herbert Xu:
Hi Herbert,
> On Mon, Jan 26, 2015 at 05:35:07AM +0100, Stephan Mueller wrote:
> > It seems I have misunderstood you in the last discussion.
>
> I thought you were limiting the receive SGL by ALG_MAX_PAGES rather
> than a single IOV entry.
Ok, I will do that. Sorry for the misunderstanding.
>
> Cheers,
--
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH v11 1/2] crypto: AF_ALG: add AEAD support
From: Herbert Xu @ 2015-01-26 4:41 UTC (permalink / raw)
To: Stephan Mueller
Cc: Daniel Borkmann, 'Quentin Gouchet', 'LKML',
linux-crypto, linux-api, Neil Horman
In-Reply-To: <1793732.EQDAUxrRn5@tachyon.chronox.de>
On Mon, Jan 26, 2015 at 05:39:27AM +0100, Stephan Mueller wrote:
>
> But does it really matter if we consider size == 0 or != at this point? In
> case size == 0, the len calculation before the inner while loop will return 0.
Of course it matters because you may die due to the aead_writable
check.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/4] IB/uverbs: ex_query_device: check request's comp_mask
From: Yann Droneaud @ 2015-01-26 11:17 UTC (permalink / raw)
To: Haggai Eran
Cc: Sagi Grimberg, Shachar Raindel, Eli Cohen, Roland Dreier,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <54C50A67.6040306-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Hi,
(adding linux-api@ for comments:
We're introducing a new "uverb" in the InfiniBand subsystem:
extended QUERY_DEVICE in v3.19:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/core/uverbs_cmd.c?id=v3.19-rc6#n3297
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/rdma/ib_user_verbs.h?id=v3.19-rc6#n209
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/rdma/ib_user_verbs.h?id=v3.19-rc6#n224
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a77abf9a97a
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=860f10a799c8
As you may not already know, InfiniBand subsystem use a write()
syscall[1] to issue ioctl()-like operations. Many operations (aka
verbs) are available [2], for each there's a query data structures
and for some there's a response data structure [3]. As a result to a
write() operation, kernel could allocate resources on the task behalf
and/or write data back to userspace provided buffers whose pointers
were part of buffer passed to write().
I've expressed concern on the way the new extended QUERY_DEVICE
was trying to be itself expandable: by silently ignoring shorter
buffer, returning truncated data, the interface seems awkward.
"Re: [PATCH v2 06/17] IB/core: Add support for extended query device caps"
http://mid.gmane.org/1418216676.11111.45.camel-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
"Re: [PATCH v3 06/17] IB/core: Add support for extended query device caps"
http://mid.gmane.org/1418733236.2779.26.camel-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/1418824811.3334.3.camel@dworkin
http://mid.gmane.org/1421844612.13543.40.camel-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
I recognize the author's intention to provide a way for userspace
to retrieve easily the highest supported information as something
desirable.
But I believe we must be more strict on the request content and
fail for any unrecognized, unsupported, incorrect bits to make
safer to extend the interface latter.
I've submitted a patchset[4] to address these issues.
But, while I'm convinced it the way to go, I'm not able to find
how it could be made to satisfy the original author expectations.
I hope linux-api@ readers could provide some insight regarding
the way we should implement such interface
[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/core/uverbs_main.c?id=v3.19-rc6#n599
[2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/core/uverbs_main.c?id=v3.19-rc6#n81
[3] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/rdma/ib_user_verbs.h?id=v3.19-rc6
[4] http://mid.gmane.org/cover.1421931555.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
)
Le dimanche 25 janvier 2015 à 17:23 +0200, Haggai Eran a écrit :
> On 22/01/2015 15:28, Yann Droneaud wrote:
> > This patch ensures the extended QUERY_DEVICE uverbs request's
> > comp_mask has only known values. If userspace returns unknown
> > features, -EINVAL will be returned, allowing to probe/discover
> > which features are currently supported by the kernel.
>
> This probing process will be much more cumbersome than it needs to be
> because userspace will have to call QUERY_DEVICE repetitively with
> different comp_mask fields until it finds out the exact set of supported
> bits.
>
O(log2(N))
Or you had to add a different interface, dedicated to retrieve the exact
supported feature mask.
> > Moreover, it also ensure the requested features set in comp_mask
> > are sequentially set, not skipping intermediate features, eg. the
> > "highest" requested feature also request all the "lower" ones.
> > This way each new feature will have to be stacked on top of the
> > existing ones: this is especially important for the request and
> > response data structures where fields are added after the
> > current ones when expanded to support a new feature.
>
> I think it is perfectly acceptable that not all drivers will implement
> all available features, and so you shouldn't enforce this requirement.
With regard to QUERY_DEVICE: the data structure layout depends on the
comp_mask value. So either you propose a way to express multipart data
structure (see CMSG or NETLINK), or we have to ensure the data structure
is ever-growing, with each new chunck stacked over the existing ones:
that's the purpose of :
if (cmd.comp_mask & (cmd.comp_mask + 1))
return -EINVAL;
> Also, it makes the comp_mask nothing more than a wasteful version number
> between 0 and 63.
That's what I've already explained earlier in "Re: [PATCH v3 06/17]
IB/core: Add support for extended query device caps":
http://mid.gmane.org/1421844612.13543.40.camel-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>
> In the specific case of QUERY_DEVICE you might argue that there isn't
> any need for input comp_mask, only for output, and then you may enforce
> the input comp_mask will always be zero.
The extended QUERY_DEVICE uverbs as currently merged is using comp_mask
from input to choose to report on-demand-paging related value. So it
seems it's needed.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/core/uverbs_cmd.c?id=v3.19-rc6#n3297
> However, you will in any case need to be able to extended the size of the response in the future.
>
That's already the case for on demand paging.
> >
> > Link: http://mid.gmane.org/cover.1421931555.git.ydroneaud-RlY5vtjFyJ1hl2p70BpVqQ@public.gmane.orgm
> > 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 | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> > index 8668b328b7e6..80a1c90f1dbf 100644
> > --- a/drivers/infiniband/core/uverbs_cmd.c
> > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > @@ -3313,6 +3313,12 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
> > if (err)
> > return err;
> >
> > + if (cmd.comp_mask & (cmd.comp_mask + 1))
> > + return -EINVAL;
> > +
> > + if (cmd.comp_mask & ~(__u32)IB_USER_VERBS_EX_QUERY_DEVICE_ODP)
> > + return -EINVAL;
> > +
If we keep the checks on output buffer size from patch 1, returning
-ENOSPC in case of size mismatch, and if we are sure that no bit in
input comp_mask will ever trigger a call to a kernel function that can
make the uverb fail, the latter check on known value could be dropped,
allowing the userspace to set the highest mask (-1): userspace
will use -ENOSPC to probe the expected size of the response buffer
to match the highest supported comp_mask. But it's going to hurt
userspace application not ready to receive -ENOSPC on newer kernel
with extended QUERY_DEVICE ABI ... Oops.
So in this end, the safest way to ensure userspace is doing the correct
thing so that we have backward and forward compatibility is to check
for known values in comp_mask, check for response buffer size and ensure
that data structure chunk are stacked.
The tighter are the checks now, the easier the interface could be
extended latter.
> > if (cmd.reserved)
> > return -EINVAL;
> >
> >
>
Regards.
--
Yann Droneaud
OPTEYA
--
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
* [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API
From: Mauro Carvalho Chehab @ 2015-01-26 12:47 UTC (permalink / raw)
To: Linux Media Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, Antti Palosaari, Ricardo Ribalda, Marek Szyprowski,
Ramakrishnan Muthukrishnan, Laurent Pinchart, linux-api
In-Reply-To: <cover.1422273497.git.mchehab@osg.samsung.com>
The previous provision for DVB media controller support were to
define an ID (likely meaning the adapter number) for the DVB
devnodes.
This is just plain wrong. Just like V4L, DVB devices (and ALSA,
or whatever) are identified via a (major, minor) tuple.
This is enough to uniquely identify a devnode, no matter what
API it implements.
So, before we go too far, let's mark the old v4l, dvb and alsa
"devnode" info as deprecated, and just call it as "dev".
As we don't want to break compilation on already existing apps,
let's just keep the old definitions as-is, adding a note that
those are deprecated at media-entity.h.
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 86bb93fd7db8..d89d5cb465d9 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
vdev->vfl_type != VFL_TYPE_SUBDEV) {
vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
vdev->entity.name = vdev->name;
- vdev->entity.info.v4l.major = VIDEO_MAJOR;
- vdev->entity.info.v4l.minor = vdev->minor;
+ vdev->entity.info.dev.major = VIDEO_MAJOR;
+ vdev->entity.info.dev.minor = vdev->minor;
ret = media_device_register_entity(vdev->v4l2_dev->mdev,
&vdev->entity);
if (ret < 0)
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 015f92aab44a..204cc67c84e8 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
goto clean_up;
}
#if defined(CONFIG_MEDIA_CONTROLLER)
- sd->entity.info.v4l.major = VIDEO_MAJOR;
- sd->entity.info.v4l.minor = vdev->minor;
+ sd->entity.info.dev.major = VIDEO_MAJOR;
+ sd->entity.info.dev.minor = vdev->minor;
#endif
sd->devnode = vdev;
}
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index e00459185d20..d6d74bcfe183 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -87,17 +87,7 @@ struct media_entity {
struct {
u32 major;
u32 minor;
- } v4l;
- struct {
- u32 major;
- u32 minor;
- } fb;
- struct {
- u32 card;
- u32 device;
- u32 subdevice;
- } alsa;
- int dvb;
+ } dev;
/* Sub-device specifications */
/* Nothing needed yet */
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index d847c760e8f0..418f4fec391a 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -78,6 +78,20 @@ struct media_entity_desc {
struct {
__u32 major;
__u32 minor;
+ } dev;
+
+#if 1
+ /*
+ * DEPRECATED: previous node specifications. Kept just to
+ * avoid breaking compilation, but media_entity_desc.dev
+ * should be used instead. In particular, alsa and dvb
+ * fields below are wrong: for all devnodes, there should
+ * be just major/minor inside the struct, as this is enough
+ * to represent any devnode, no matter what type.
+ */
+ struct {
+ __u32 major;
+ __u32 minor;
} v4l;
struct {
__u32 major;
@@ -89,6 +103,7 @@ struct media_entity_desc {
__u32 subdevice;
} alsa;
int dvb;
+#endif
/* Sub-device specifications */
/* Nothing needed yet */
--
2.1.0
^ permalink raw reply related
* [PATCH 2/3] media: add new types for DVB devnodes
From: Mauro Carvalho Chehab @ 2015-01-26 12:47 UTC (permalink / raw)
To: Linux Media Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-api
In-Reply-To: <cover.1422273497.git.mchehab@osg.samsung.com>
Most of the DVB subdevs have already their own devnode.
Add support for them at the media controller API.
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 418f4fec391a..4c8f26243252 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -50,7 +50,14 @@ struct media_device_info {
#define MEDIA_ENT_T_DEVNODE_V4L (MEDIA_ENT_T_DEVNODE + 1)
#define MEDIA_ENT_T_DEVNODE_FB (MEDIA_ENT_T_DEVNODE + 2)
#define MEDIA_ENT_T_DEVNODE_ALSA (MEDIA_ENT_T_DEVNODE + 3)
-#define MEDIA_ENT_T_DEVNODE_DVB (MEDIA_ENT_T_DEVNODE + 4)
+#define MEDIA_ENT_T_DEVNODE_DVB_FE (MEDIA_ENT_T_DEVNODE + 4)
+#define MEDIA_ENT_T_DEVNODE_DVB_DEMUX (MEDIA_ENT_T_DEVNODE + 5)
+#define MEDIA_ENT_T_DEVNODE_DVB_DVR (MEDIA_ENT_T_DEVNODE + 6)
+#define MEDIA_ENT_T_DEVNODE_DVB_CA (MEDIA_ENT_T_DEVNODE + 7)
+#define MEDIA_ENT_T_DEVNODE_DVB_NET (MEDIA_ENT_T_DEVNODE + 8)
+
+/* Legacy symbol. Use it to avoid userspace compilation breakages */
+#define MEDIA_ENT_T_DEVNODE_DVB MEDIA_ENT_T_DEVNODE_DVB_FE
#define MEDIA_ENT_T_V4L2_SUBDEV (2 << MEDIA_ENT_TYPE_SHIFT)
#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR (MEDIA_ENT_T_V4L2_SUBDEV + 1)
--
2.1.0
^ permalink raw reply related
* [PATCH 3/3] media: add a subdev type for tuner
From: Mauro Carvalho Chehab @ 2015-01-26 12:47 UTC (permalink / raw)
To: Linux Media Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-api
In-Reply-To: <cover.1422273497.git.mchehab@osg.samsung.com>
Add MEDIA_ENT_T_V4L2_SUBDEV_TUNER to represent the V4L2
(and dvb) tuner subdevices.
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 4c8f26243252..52cc2a6b19b7 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -66,6 +66,8 @@ struct media_device_info {
/* A converter of analogue video to its digital representation. */
#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER (MEDIA_ENT_T_V4L2_SUBDEV + 4)
+#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER (MEDIA_ENT_T_V4L2_SUBDEV + 5)
+
#define MEDIA_ENT_FL_DEFAULT (1 << 0)
struct media_entity_desc {
--
2.1.0
^ permalink raw reply related
* Re: [RFC][PATCH v2] procfs: Always expose /proc/<pid>/map_files/ and make it readable
From: Kirill A. Shutemov @ 2015-01-26 12:47 UTC (permalink / raw)
To: Calvin Owens
Cc: Cyrill Gorcunov, Andrew Morton, Alexey Dobriyan, Oleg Nesterov,
Eric W. Biederman, Al Viro, Kirill A. Shutemov, Peter Feiner,
Grant Likely, Siddhesh Poyarekar, linux-kernel, kernel-team,
Pavel Emelyanov, linux-api
In-Reply-To: <20150124031544.GA1992748@mail.thefacebook.com>
On Fri, Jan 23, 2015 at 07:15:44PM -0800, Calvin Owens wrote:
> Currently, /proc/<pid>/map_files/ is restricted to CAP_SYS_ADMIN, and
> is only exposed if CONFIG_CHECKPOINT_RESTORE is set. This interface
> is very useful for enumerating the files mapped into a process when
> the more verbose information in /proc/<pid>/maps is not needed.
>
> This patch moves the folder out from behind CHECKPOINT_RESTORE, and
> removes the CAP_SYS_ADMIN restrictions. Following the links requires
> the ability to ptrace the process in question, so this doesn't allow
> an attacker to do anything they couldn't already do before.
>
> Signed-off-by: Calvin Owens <calvinowens@fb.com>
Cc +linux-api@
> ---
> Changes in v2: Removed the follow_link() stub that returned -EPERM if
> the caller didn't have CAP_SYS_ADMIN, since the caller
> in my chroot() scenario gets -EACCES anyway.
>
> fs/proc/base.c | 18 ------------------
> 1 file changed, 18 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 3f3d7ae..67b15ac 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1632,8 +1632,6 @@ end_instantiate:
> return dir_emit(ctx, name, len, 1, DT_UNKNOWN);
> }
>
> -#ifdef CONFIG_CHECKPOINT_RESTORE
> -
> /*
> * dname_to_vma_addr - maps a dentry name into two unsigned longs
> * which represent vma start and end addresses.
> @@ -1660,11 +1658,6 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
> if (flags & LOOKUP_RCU)
> return -ECHILD;
>
> - if (!capable(CAP_SYS_ADMIN)) {
> - status = -EPERM;
> - goto out_notask;
> - }
> -
> inode = dentry->d_inode;
> task = get_proc_task(inode);
> if (!task)
> @@ -1792,10 +1785,6 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
> int result;
> struct mm_struct *mm;
>
> - result = -EPERM;
> - if (!capable(CAP_SYS_ADMIN))
> - goto out;
> -
> result = -ENOENT;
> task = get_proc_task(dir);
> if (!task)
> @@ -1849,10 +1838,6 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
> struct map_files_info *p;
> int ret;
>
> - ret = -EPERM;
> - if (!capable(CAP_SYS_ADMIN))
> - goto out;
> -
> ret = -ENOENT;
> task = get_proc_task(file_inode(file));
> if (!task)
> @@ -2040,7 +2025,6 @@ static const struct file_operations proc_timers_operations = {
> .llseek = seq_lseek,
> .release = seq_release_private,
> };
> -#endif /* CONFIG_CHECKPOINT_RESTORE */
>
> static int proc_pident_instantiate(struct inode *dir,
> struct dentry *dentry, struct task_struct *task, const void *ptr)
> @@ -2537,9 +2521,7 @@ static const struct inode_operations proc_task_inode_operations;
> static const struct pid_entry tgid_base_stuff[] = {
> DIR("task", S_IRUGO|S_IXUGO, proc_task_inode_operations, proc_task_operations),
> DIR("fd", S_IRUSR|S_IXUSR, proc_fd_inode_operations, proc_fd_operations),
> -#ifdef CONFIG_CHECKPOINT_RESTORE
> DIR("map_files", S_IRUSR|S_IXUSR, proc_map_files_inode_operations, proc_map_files_operations),
> -#endif
> DIR("fdinfo", S_IRUSR|S_IXUSR, proc_fdinfo_inode_operations, proc_fdinfo_operations),
> DIR("ns", S_IRUSR|S_IXUGO, proc_ns_dir_inode_operations, proc_ns_dir_operations),
> #ifdef CONFIG_NET
> --
> 1.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API
From: Hans Verkuil @ 2015-01-26 13:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Media Mailing List
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
Antti Palosaari, Ricardo Ribalda, Marek Szyprowski,
Ramakrishnan Muthukrishnan, Laurent Pinchart, linux-api
In-Reply-To: <cb0517f150942a2d3657c1f2e55754061bfae2c4.1422273497.git.mchehab@osg.samsung.com>
On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
> The previous provision for DVB media controller support were to
> define an ID (likely meaning the adapter number) for the DVB
> devnodes.
>
> This is just plain wrong. Just like V4L, DVB devices (and ALSA,
> or whatever) are identified via a (major, minor) tuple.
>
> This is enough to uniquely identify a devnode, no matter what
> API it implements.
>
> So, before we go too far, let's mark the old v4l, dvb and alsa
> "devnode" info as deprecated, and just call it as "dev".
>
> As we don't want to break compilation on already existing apps,
> let's just keep the old definitions as-is, adding a note that
> those are deprecated at media-entity.h.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 86bb93fd7db8..d89d5cb465d9 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
> vdev->vfl_type != VFL_TYPE_SUBDEV) {
> vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
> vdev->entity.name = vdev->name;
> - vdev->entity.info.v4l.major = VIDEO_MAJOR;
> - vdev->entity.info.v4l.minor = vdev->minor;
> + vdev->entity.info.dev.major = VIDEO_MAJOR;
> + vdev->entity.info.dev.minor = vdev->minor;
> ret = media_device_register_entity(vdev->v4l2_dev->mdev,
> &vdev->entity);
> if (ret < 0)
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 015f92aab44a..204cc67c84e8 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
> goto clean_up;
> }
> #if defined(CONFIG_MEDIA_CONTROLLER)
> - sd->entity.info.v4l.major = VIDEO_MAJOR;
> - sd->entity.info.v4l.minor = vdev->minor;
> + sd->entity.info.dev.major = VIDEO_MAJOR;
> + sd->entity.info.dev.minor = vdev->minor;
> #endif
> sd->devnode = vdev;
> }
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e00459185d20..d6d74bcfe183 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -87,17 +87,7 @@ struct media_entity {
> struct {
> u32 major;
> u32 minor;
> - } v4l;
> - struct {
> - u32 major;
> - u32 minor;
> - } fb;
> - struct {
> - u32 card;
> - u32 device;
> - u32 subdevice;
> - } alsa;
I don't think the alsa entity information can be replaced by major/minor.
In particular you will loose the subdevice information which you need as
well. In addition, alsa devices are almost never referenced via major and
minor numbers, but always by card/device/subdevice numbers.
> - int dvb;
> + } dev;
>
> /* Sub-device specifications */
> /* Nothing needed yet */
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index d847c760e8f0..418f4fec391a 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -78,6 +78,20 @@ struct media_entity_desc {
> struct {
> __u32 major;
> __u32 minor;
> + } dev;
> +
> +#if 1
> + /*
> + * DEPRECATED: previous node specifications. Kept just to
> + * avoid breaking compilation, but media_entity_desc.dev
> + * should be used instead. In particular, alsa and dvb
> + * fields below are wrong: for all devnodes, there should
> + * be just major/minor inside the struct, as this is enough
> + * to represent any devnode, no matter what type.
> + */
> + struct {
> + __u32 major;
> + __u32 minor;
> } v4l;
> struct {
> __u32 major;
> @@ -89,6 +103,7 @@ struct media_entity_desc {
> __u32 subdevice;
> } alsa;
> int dvb;
I wouldn't merge all the v4l/fb/etc. structs into one struct. That will make it
difficult in the future if you need to add a field for e.g. v4l entities.
So I would keep the v4l, fb and alsa structs, and just add a new struct for
dvb. I wonder if the dvb field can't just be replaced since I doubt anyone is
using it. And even if someone does, then it can't be right since a single
int isn't enough and never worked anyway.
Regards,
Hans
> +#endif
>
> /* Sub-device specifications */
> /* Nothing needed yet */
>
^ permalink raw reply
* Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API
From: Mauro Carvalho Chehab @ 2015-01-26 13:34 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, Antti Palosaari, Ricardo Ribalda, Marek Szyprowski,
Ramakrishnan Muthukrishnan, Laurent Pinchart, linux-api
In-Reply-To: <54C63D16.3070607@xs4all.nl>
Em Mon, 26 Jan 2015 14:11:50 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
> > The previous provision for DVB media controller support were to
> > define an ID (likely meaning the adapter number) for the DVB
> > devnodes.
> >
> > This is just plain wrong. Just like V4L, DVB devices (and ALSA,
> > or whatever) are identified via a (major, minor) tuple.
> >
> > This is enough to uniquely identify a devnode, no matter what
> > API it implements.
> >
> > So, before we go too far, let's mark the old v4l, dvb and alsa
> > "devnode" info as deprecated, and just call it as "dev".
> >
> > As we don't want to break compilation on already existing apps,
> > let's just keep the old definitions as-is, adding a note that
> > those are deprecated at media-entity.h.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 86bb93fd7db8..d89d5cb465d9 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
> > vdev->vfl_type != VFL_TYPE_SUBDEV) {
> > vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
> > vdev->entity.name = vdev->name;
> > - vdev->entity.info.v4l.major = VIDEO_MAJOR;
> > - vdev->entity.info.v4l.minor = vdev->minor;
> > + vdev->entity.info.dev.major = VIDEO_MAJOR;
> > + vdev->entity.info.dev.minor = vdev->minor;
> > ret = media_device_register_entity(vdev->v4l2_dev->mdev,
> > &vdev->entity);
> > if (ret < 0)
> > diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> > index 015f92aab44a..204cc67c84e8 100644
> > --- a/drivers/media/v4l2-core/v4l2-device.c
> > +++ b/drivers/media/v4l2-core/v4l2-device.c
> > @@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
> > goto clean_up;
> > }
> > #if defined(CONFIG_MEDIA_CONTROLLER)
> > - sd->entity.info.v4l.major = VIDEO_MAJOR;
> > - sd->entity.info.v4l.minor = vdev->minor;
> > + sd->entity.info.dev.major = VIDEO_MAJOR;
> > + sd->entity.info.dev.minor = vdev->minor;
> > #endif
> > sd->devnode = vdev;
> > }
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index e00459185d20..d6d74bcfe183 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -87,17 +87,7 @@ struct media_entity {
> > struct {
> > u32 major;
> > u32 minor;
> > - } v4l;
> > - struct {
> > - u32 major;
> > - u32 minor;
> > - } fb;
> > - struct {
> > - u32 card;
> > - u32 device;
> > - u32 subdevice;
> > - } alsa;
>
> I don't think the alsa entity information can be replaced by major/minor.
> In particular you will loose the subdevice information which you need as
> well. In addition, alsa devices are almost never referenced via major and
> minor numbers, but always by card/device/subdevice numbers.
For media-ctl, it is easier to handle major/minor, in order to identify
the associated devnode name. Btw, media-ctl currently assumes that all
devnode devices are specified by v4l.major/v4l.minor.
Ok, maybe for alsa we'll need also card/device/subdevice, but I think this
should be mapped elsewhere, if this can't be retrieved via its sysfs/udev
interface (with seems to be doubtful).
>
> > - int dvb;
> > + } dev;
> >
> > /* Sub-device specifications */
> > /* Nothing needed yet */
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index d847c760e8f0..418f4fec391a 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -78,6 +78,20 @@ struct media_entity_desc {
> > struct {
> > __u32 major;
> > __u32 minor;
> > + } dev;
> > +
> > +#if 1
> > + /*
> > + * DEPRECATED: previous node specifications. Kept just to
> > + * avoid breaking compilation, but media_entity_desc.dev
> > + * should be used instead. In particular, alsa and dvb
> > + * fields below are wrong: for all devnodes, there should
> > + * be just major/minor inside the struct, as this is enough
> > + * to represent any devnode, no matter what type.
> > + */
> > + struct {
> > + __u32 major;
> > + __u32 minor;
> > } v4l;
> > struct {
> > __u32 major;
> > @@ -89,6 +103,7 @@ struct media_entity_desc {
> > __u32 subdevice;
> > } alsa;
> > int dvb;
>
> I wouldn't merge all the v4l/fb/etc. structs into one struct. That will make it
> difficult in the future if you need to add a field for e.g. v4l entities.
No. You could just create another union for the API-specific bits, using the
reserved bytes.
> So I would keep the v4l, fb and alsa structs, and just add a new struct for
> dvb. I wonder if the dvb field can't just be replaced since I doubt anyone is
> using it. And even if someone does, then it can't be right since a single
> int isn't enough and never worked anyway.
All devnodes have major/minor. Making it standard for all devices makes
easy for userspace to properly get the data it requires to work.
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API
From: Hans Verkuil @ 2015-01-26 13:41 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, Antti Palosaari, Ricardo Ribalda, Marek Szyprowski,
Ramakrishnan Muthukrishnan, Laurent Pinchart,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150126113416.311fb376-+RedX5hVuTR+urZeOPWqwQ@public.gmane.org>
On 01/26/2015 02:34 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 26 Jan 2015 14:11:50 +0100
> Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> escreveu:
>
>> On 01/26/2015 01:47 PM, Mauro Carvalho Chehab wrote:
>>> The previous provision for DVB media controller support were to
>>> define an ID (likely meaning the adapter number) for the DVB
>>> devnodes.
>>>
>>> This is just plain wrong. Just like V4L, DVB devices (and ALSA,
>>> or whatever) are identified via a (major, minor) tuple.
>>>
>>> This is enough to uniquely identify a devnode, no matter what
>>> API it implements.
>>>
>>> So, before we go too far, let's mark the old v4l, dvb and alsa
>>> "devnode" info as deprecated, and just call it as "dev".
>>>
>>> As we don't want to break compilation on already existing apps,
>>> let's just keep the old definitions as-is, adding a note that
>>> those are deprecated at media-entity.h.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index 86bb93fd7db8..d89d5cb465d9 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -943,8 +943,8 @@ int __video_register_device(struct video_device *vdev, int type, int nr,
>>> vdev->vfl_type != VFL_TYPE_SUBDEV) {
>>> vdev->entity.type = MEDIA_ENT_T_DEVNODE_V4L;
>>> vdev->entity.name = vdev->name;
>>> - vdev->entity.info.v4l.major = VIDEO_MAJOR;
>>> - vdev->entity.info.v4l.minor = vdev->minor;
>>> + vdev->entity.info.dev.major = VIDEO_MAJOR;
>>> + vdev->entity.info.dev.minor = vdev->minor;
>>> ret = media_device_register_entity(vdev->v4l2_dev->mdev,
>>> &vdev->entity);
>>> if (ret < 0)
>>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>>> index 015f92aab44a..204cc67c84e8 100644
>>> --- a/drivers/media/v4l2-core/v4l2-device.c
>>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>>> @@ -248,8 +248,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>>> goto clean_up;
>>> }
>>> #if defined(CONFIG_MEDIA_CONTROLLER)
>>> - sd->entity.info.v4l.major = VIDEO_MAJOR;
>>> - sd->entity.info.v4l.minor = vdev->minor;
>>> + sd->entity.info.dev.major = VIDEO_MAJOR;
>>> + sd->entity.info.dev.minor = vdev->minor;
>>> #endif
>>> sd->devnode = vdev;
>>> }
>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>> index e00459185d20..d6d74bcfe183 100644
>>> --- a/include/media/media-entity.h
>>> +++ b/include/media/media-entity.h
>>> @@ -87,17 +87,7 @@ struct media_entity {
>>> struct {
>>> u32 major;
>>> u32 minor;
>>> - } v4l;
>>> - struct {
>>> - u32 major;
>>> - u32 minor;
>>> - } fb;
>>> - struct {
>>> - u32 card;
>>> - u32 device;
>>> - u32 subdevice;
>>> - } alsa;
>>
>> I don't think the alsa entity information can be replaced by major/minor.
>> In particular you will loose the subdevice information which you need as
>> well. In addition, alsa devices are almost never referenced via major and
>> minor numbers, but always by card/device/subdevice numbers.
>
> For media-ctl, it is easier to handle major/minor, in order to identify
> the associated devnode name. Btw, media-ctl currently assumes that all
> devnode devices are specified by v4l.major/v4l.minor.
>
> Ok, maybe for alsa we'll need also card/device/subdevice, but I think this
> should be mapped elsewhere, if this can't be retrieved via its sysfs/udev
> interface (with seems to be doubtful).
The card/device tuple can likely be mapped to major/minor, but not subdevice.
And since everything inside alsa is based on card/device I wouldn't change
that.
>
>>
>>> - int dvb;
>>> + } dev;
>>>
>>> /* Sub-device specifications */
>>> /* Nothing needed yet */
>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>> index d847c760e8f0..418f4fec391a 100644
>>> --- a/include/uapi/linux/media.h
>>> +++ b/include/uapi/linux/media.h
>>> @@ -78,6 +78,20 @@ struct media_entity_desc {
>>> struct {
>>> __u32 major;
>>> __u32 minor;
>>> + } dev;
>>> +
>>> +#if 1
>>> + /*
>>> + * DEPRECATED: previous node specifications. Kept just to
>>> + * avoid breaking compilation, but media_entity_desc.dev
>>> + * should be used instead. In particular, alsa and dvb
>>> + * fields below are wrong: for all devnodes, there should
>>> + * be just major/minor inside the struct, as this is enough
>>> + * to represent any devnode, no matter what type.
>>> + */
>>> + struct {
>>> + __u32 major;
>>> + __u32 minor;
>>> } v4l;
>>> struct {
>>> __u32 major;
>>> @@ -89,6 +103,7 @@ struct media_entity_desc {
>>> __u32 subdevice;
>>> } alsa;
>>> int dvb;
>>
>> I wouldn't merge all the v4l/fb/etc. structs into one struct. That will make it
>> difficult in the future if you need to add a field for e.g. v4l entities.
>
> No. You could just create another union for the API-specific bits, using the
> reserved bytes.
>
>> So I would keep the v4l, fb and alsa structs, and just add a new struct for
>> dvb. I wonder if the dvb field can't just be replaced since I doubt anyone is
>> using it. And even if someone does, then it can't be right since a single
>> int isn't enough and never worked anyway.
>
> All devnodes have major/minor. Making it standard for all devices makes
> easy for userspace to properly get the data it requires to work.
I think you are making assumptions here that may not be true. I don't see any
reason to make a 'dev' struct here. The real problem is the dvb int, so that's
what needs to be addressed. Changing anything else will cause API headaches
for no good reason.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API
From: Devin Heitmueller @ 2015-01-26 14:00 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, Linux Media Mailing List, Mauro Carvalho Chehab,
Hans Verkuil, Sakari Ailus, Antti Palosaari, Ricardo Ribalda,
Marek Szyprowski, Ramakrishnan Muthukrishnan, Laurent Pinchart,
linux-api
In-Reply-To: <20150126113416.311fb376@recife.lan>
> For media-ctl, it is easier to handle major/minor, in order to identify
> the associated devnode name. Btw, media-ctl currently assumes that all
> devnode devices are specified by v4l.major/v4l.minor.
I suspect part of the motivation for the "id" that corresponds to the
adapter field was to make it easier to find the actual underlying
device node. While it's trivial to convert a V4L device node from
major/minor to the device node (since for major number is constant and
the minor corresponds to the X in /dev/videoX), that's tougher with
DVB adapters because of the hierarchical nature of the DVB device
nodes. Having the adapter number makes it trivial to open
/dev/dvb/adapterX.
Perhaps my POSIX is rusty -- is there a way to identify the device
node based on major minor without having to traverse the entire /dev
tree?
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
^ permalink raw reply
* Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API
From: Mauro Carvalho Chehab @ 2015-01-26 14:31 UTC (permalink / raw)
To: Devin Heitmueller
Cc: Hans Verkuil, Linux Media Mailing List, Mauro Carvalho Chehab,
Hans Verkuil, Sakari Ailus, Antti Palosaari, Ricardo Ribalda,
Marek Szyprowski, Ramakrishnan Muthukrishnan, Laurent Pinchart,
linux-api
In-Reply-To: <CAGoCfixoSxspEzpCB95BVPXBrZr2gpDVWHbaikESsuB1V=WM1g@mail.gmail.com>
Em Mon, 26 Jan 2015 09:00:46 -0500
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:
> > For media-ctl, it is easier to handle major/minor, in order to identify
> > the associated devnode name. Btw, media-ctl currently assumes that all
> > devnode devices are specified by v4l.major/v4l.minor.
>
> I suspect part of the motivation for the "id" that corresponds to the
> adapter field was to make it easier to find the actual underlying
> device node.
Yes, that was the reason why, back in 2007, we believed that just id
would be enough. Yet, we never tried to implement it, until the end
of the last year.
> While it's trivial to convert a V4L device node from
> major/minor to the device node (since for major number is constant and
> the minor corresponds to the X in /dev/videoX), that's tougher with
> DVB adapters because of the hierarchical nature of the DVB device
> nodes. Having the adapter number makes it trivial to open
> /dev/dvb/adapterX.
>
> Perhaps my POSIX is rusty -- is there a way to identify the device
> node based on major minor without having to traverse the entire /dev
> tree?
It is actually trivial to get the device nodes once you have the
major/minor. The media-ctl library does that for you. See:
$ media-ctl --print-dot
digraph board {
rankdir=TB
n00000001 [label="{{<port0> 0} | cx25840 19-0044 | {<port1> 1 | <port2> 2}}", shape=Mrecord, style=filled, fillcolor=green]
n00000001:port1 -> n00000003
n00000001:port2 -> n00000004
n00000002 [label="{{} | NXP TDA18271HD | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
n00000002:port0 -> n00000005 [style=dashed]
n00000002:port0 -> n00000001:port0
n00000003 [label="cx231xx #0 video\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
n00000004 [label="cx231xx #0 vbi\n/dev/vbi0", shape=box, style=filled, fillcolor=yellow]
n00000005 [label="Fujitsu mb86A20s\n/dev/dvb/adapter0/frontend0", shape=box, style=filled, fillcolor=yellow]
n00000005 -> n00000006
n00000006 [label="demux\n/dev/dvb/adapter0/demux0", shape=box, style=filled, fillcolor=yellow]
n00000006 -> n00000007
n00000007 [label="dvr\n/dev/dvb/adapter0/dvr0", shape=box, style=filled, fillcolor=yellow]
n00000008 [label="dvb net\n/dev/dvb/adapter0/net0", shape=box, style=filled, fillcolor=yellow]
}
There are two routines inside the media-ctl libraries to convert from
major/minor into a devnode name like: /dev/dvb/adapter0/demux0.
The first one uses sysfs:
$ ls -la /sys/dev/char|grep dvb
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:0 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.frontend0
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:1 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.demux0
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:2 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.dvr0
lrwxrwxrwx. 1 root root 0 Jan 26 10:32 212:3 -> ../../devices/pci0000:00/0000:00:14.0/usb1/1-2/1-2:1.1/dvb/dvb0.net0
Unfortunately, the sysfs nodes are "dvb0" for adapter0, so a patch is needed
to fix it:
http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/commit/?h=dvb-media-ctl&id=d854a9bb24706dbfc878484e4538d79b1ac52aae
The second (and better) approach is to require udev to return the name of the
devnode. The logic, implemented at utils/media-ctl/libmediactl.c, inside
v4l-utils, is:
devnum = makedev(entity->info.v4l.major, entity->info.v4l.minor);
media_dbg(entity->media, "looking up device: %u:%u\n",
major(devnum), minor(devnum));
device = udev_device_new_from_devnum(udev, 'c', devnum);
Right now, by default, media-ctl will use the sysfs approach, except
if an extra option is called at ./configure, in order to enable it to
use the udev library.
IMHO, we should make udev the default behavior, if libudev-dev(el) is
there at compilation time.
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH 1/3] media: Fix ALSA and DVB representation at media controller API
From: Devin Heitmueller @ 2015-01-26 14:41 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, Linux Media Mailing List, Mauro Carvalho Chehab,
Hans Verkuil, Sakari Ailus, Antti Palosaari, Ricardo Ribalda,
Marek Szyprowski, Ramakrishnan Muthukrishnan, Laurent Pinchart,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150126123129.2076b9f8-+RedX5hVuTR+urZeOPWqwQ@public.gmane.org>
> It is actually trivial to get the device nodes once you have the
> major/minor. The media-ctl library does that for you. See:
No objection then.
On a related note, you would be very well served to consider testing
your dvb changes with a device that has more than one DVB tuner (such
as the hvr-2200/2250). That will help you shake out any edge cases
related to ensuring that the different DVB nodes appear in different
groups.
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Michael Kerrisk (man-pages) @ 2015-01-26 14:42 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, Daniel Mack,
arnd-r2nGTMty4D4, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io, teg-B22kvLQNl6c,
jkosina-AlSwsSmVLrQ, luto-kltTT9wpgjJwATOyAt5JVQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w, tixxdz-Umm1ozX2/EEdnm+yROfE0A,
Johannes Stezenbach, Theodore T'so, christoph Hellwig
In-Reply-To: <20150123155402.GB2159-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Hi Greg,
First of all, I seem to have offended you. That was not my intention.
It's certainly not my intent to disparage you or your work (or for
that matter, the other kdbus developers). Insofar as any of the wordings
I've used suggested otherwise, I do apologize.
I'll comment on various points below, keeping it as technical as I can.
Then I have a couple of general questions at the end with the goal
of ensuring that my comments are not coming from a broken world view.
On 01/23/2015 04:54 PM, Greg Kroah-Hartman wrote:
> On Thu, Jan 22, 2015 at 11:18:50AM +0100, Michael Kerrisk (man-pages) wrote:
>>>> And that process seems to be frequent and ongoing even now. (And
>>>> it's to your great credit that the API/ABI breaks are clearly and honestly
>>>> marked in the kdbus.h changelog.) All of this lightens the burden of API
>>>> design for kernel developers, but I'm concerned that the long-term pain
>>>> for user-space developers who use an API which (in my estimation) may
>>>> come to be widely used will be enormous.
>>>
>>> Yes, we've jointly reviewed the API details again until just recently to
>>> unify structs and enums etc, and added fields to make the ioctls structs
>>> more versatile for possible future additions. By that, we effectively
>>> broke the ABI, but we did that because we know we can't do such things
>>> again in the future.
>>>
>>> But again - I don't see how this would be different when using syscalls
>>> rather than ioctls to transport information between the driver and
>>> userspace. Could you elaborate?
>>
>> My suspicion is that not nearly enough thinking has yet been done about
>> the design of the API. That's based on these observations:
>>
>> * Documentation that is, considering the size of the API, *way* too thin.
>> * Some parts of the API not documented at all (various kdbus_item blobs)
>> * ABI changes happening even quite recently
>> * API oddities such as the 'kernel_flags' fields. Why do I need to
>> be told what flags the kernel supports on *every* operation?
>>
>> The above is just after a day of looking hard at kdbus.txt. I strongly
>> suspect I'd find a lot of other issues if I spent more time on kdbus.
>
> "not enough thinking"?
>
> We started working on kdbus 2 years ago this FOSDEM (in a few weeks.)
> Before that we have been thinking about this for many years, learning
> from the previous attempts to get this type of feature merged into the
> kernel, talking with users about what they need for this, and soliciting
> kernel developer's opinions on what type of API would be best for this
> type of feature.
>
> Since then we have done nothing but constantly revise the API. My first
> mock ups were way too simple, and in discussing things with people much
> more knowledgeable about D-Bus, they pointed out the problems, and we
> iterated. And iterated. And iterated some more. We have worked with
> just about every userspace libdbus developer group, including QtDbus
> developers as well as glib developers. Now not all of them agreed with
> some of our decisions in the implementation, which is fair enough, you
> can't please everyone, but they _all_ agree that what we have now is the
> proper way to implement this type of functionality and have reviewed the
> features as being correct and compatible with their needs and users.
>
> Those discussions have happened in emails, presentations, meetings, and
> hackfests pretty much continuously for the past 2 years all around the
> world.
>
> We have stress-tested the api with both unit tests (which are included
> here in the patch set) as well as a real-world implementation (sd-bus in
> the systemd source repo.) That real-world implementation successfully
> has been booting many of our daily machines for many months now.
Notwithstanding that I don't see how a unit test stress tests an API
*design*, I've no reason to doubt that kdbus works. But that's not the
point of my concern. I worry how usable this API is going to be for the
world at large.
> Yes, the documentation can always be better, but please don't confuse
> the lack of understanding how D-Bus works and its model with the lack of
> understanding this kdbus implementation, the two are not comparable.
> For some good primers on what D-Bus is, and the terminology it expects
> see:
> http://dbus.freedesktop.org/doc/dbus-tutorial.html
> and also:
> http://dbus.freedesktop.org/doc/dbus-faq.html#other-ipc
>
> We are not going to put a basic "here is what D-Bus is and how to use
> it" into the kernel tree, that is totally outside the scope here.
I didn't expect that you should do that. But it does touch on a general
question that I'll leave to the end of this mail.
> I suggest reading the tutorial above, and then going back and reading
> the kdbus documentation provided. If you think we are lacking stuff on
> the kdbus side, we will be glad to flush out any needed areas.
>
> Also, Daniel has said he will work on a basic userspace "example"
> library to show how to use this api, which might make the api a bit
> easier to understand.
>
> However, I personally don't think this "example code" is necessary at
> all. We don't ask for this type of "simple examples" from other new
> kernel apis we create and add to the kernel all the time. We require
> there to be a user of the api, but not one that is so well documented
> that others can write a from-scratch raw userspace replacement.
What you've just summarized there is how low a bar we've historically
set in API design. Thus, the API is littered with (for example) dozens
of system calls that were insufficiently well thought out in their
design, and have subsequently been superseded by replacements that
fixed the design mistakes. One of the cause of that problems is the
targeting of "*a* user of the API"--general purpose APIs need to be
considered from the point of view of multiple potentially different
use cases. And I'm certainly not talking about being able to reimplement
the API, But, the API is a contract, and it needs to be well understood
by its creators and consumers in order that they can assess and use that
API. Extensive Documentation generally is the best way to do that.
And, anyway, I had understood that there was a rough consensus that we do
want to see more tests/examples and documentation happening in the future.
Certainly, the number kernel developers who are taking a shot at
writing man pages these days is refreshing. (We are 7/7 for man-page
documented system calls in the 3.17-3.19 frame. That's a trend I've done
my best to encourage, and hope to see continue in the future.) And now
we have kselftest, in part thanks to your good efforts.
> Specific examples of this are my previously mentioned ioctl users
> (btrfs, mei, mic, openvlan, etc.), and the grand-daddy of all horrid
> apis, DRM.
You've made this comparison a number of times, but I think it misses
a crucial point. Those examples are all(?) domain-specific APIs with
relatively few users in terms of user-space applications, whereas,
IIUC, kdbus is intended to be an IPC mechanism that can be employed
by user-space applications in a general-purpose fashion, and upon
which potentially a multitude of different applications might be built.
That's why I think the decision to use an ioctl()-based interface
needs to be considered from a (very) skeptical point of view: no other
general-purpose IPC mechanism employs such an approach. (Again, see
one of my general questions at the end of this mail.)
> Users aren't going to be writing their own "raw kdbus" libraries. Or if
> they are, they are going to start with one of the existing
> implementations we have (the test examples and sd-bus, and I think there
> is a native Go implementation somewhere as well.) Users are going to be
> using those libraries to write their code, and to be honest, the user
> api for sd-bus is a delight to use compared to the "old style" libdbus
> interface as we have the benefit of 10 years of experience working with
> D-Bus apis in the wild now to learn from past mistakes.
>
> Back to the API. We have taken review comments on the previous postings
> of the code and reworked the API, moving it from a character device to
> be a filesystem, which ended up making things a lot easier in the end, a
> good example of a review process that is working. Those changes are
> a sign that our development review process works. People pointed out
> problems in our character api that we hadn't thought about from the
> kernel implementation side. And so we changed them and the code is
> better and more robust because of it, a success story for our review
> process.
>
> Personally, I was the one that started down the character device node
> path, so blame that design decision on me, not the other developers
> here. And I was wrong with that, but moving from character to a
> filesystem wasn't a huge change, the structures and interactions all
> remained almost identical, as the logic behind the API is, in my
> opinion, correct for the problem it is addressing.
>
> The 37 different developers who have contributed to this code base are
> quite talented and skilled and experienced in user and kernel apis,
> having implemented many apis of their own that users rely on every day.
Yes, but I am not sure that the 15 developers who made each made 1 commit
(out of 2816 to date) would have done much work on the API. And probably
the same is true for the 9 more who made just 2 or 3 commits. As one
would expect, the great deal of the good work has been done by a small
core: just shy of 95% commits by the top 5 committers.
> Yes, we all make design mistakes, and you might not agree with some of
> them, that's fine. But it is flat out rude to say that we have not been
> thinking about this, when I would guess that this is one of the largest
> (in time and contributions) kernel development feature to be worked on
> in the past few years.
>
> And yes, I'm being very defensive, as I take this very seriously, so
> please, don't so lightly dismiss us as not knowing what we are doing, as
> frankly, we do.
Greg, I did not say you hadn't been thinking about this [API design].
(But I acknowledge my words could have been better chosen.) However,
API design is hard to get right, and causes endless pain when it's wrong.
And by now I've been watching long enough to know that the mistakes
are frequent. Even Davide Libenzi, who once upon a time was one of our
more prolific and talented creators of APIs made mistakes. Thus, for
example, as we speak, the third iteration of epoll_wait() is in
development (epoll_wait() ==> epoll_pwait() ==> epoll_pwait1()).
And epoll is an API that is significantly simpler than kdbus.
In my observation, a good API design is a well documented API design.
Otherwise, it's virtually impossible to think thoroughly about the API,
and that is especially true as the API gets larger. (And AFAICS, the
kdbus API is bigger than, for example, the epoll API by an order of
magnitude, or so.) Part of that documentation also should include some
concrete examples of the use of the API. Again because it helps people
to think about and assess the API. (This is especially the case for
the fresh minds that explore the new API for the first time without
having the preconceptions that are almost inevitable for the creators
of the API.)
(BTW, I'm not ignoring your contents about the D-Bus spec above. But
kdbus is a free-standing API, IIUC, and as such, it should be assessed
on its own.)
I would summarize your statement above, as "trust us, we know what
we're doing". With respect, my default position is not to trust. It's
nothing personal: API design is hard, and mistakes are too often made.
What I want to say in return is "trust us", where "us" is used very
inclusively to mean: the kernel maintainers, and for that matter
user-space programmers, who need enough information that we can make a
well-informed assessment of the merits of an API that will need to be
supported forever and may have a multitude of different users. In
my assessment, the current information is far from sufficient, and
it's a considerable risk to merge the API lacking such information.
> Thanks for making it this far, I'll go back to technical discussions of
> the API now, as that's what we should be doing, not casting aspirations
> as to the aptitude of the people involved.
Greg, that comment, with its implication that I am not concerned about
technical matters, but rather with something more malicious was quite
uncalled for.
But, I am happy to return to technical matters. And I think it best to
start with a couple of fundamental questions, since some of the comments
I've seen to date from different kdbus developers seem to conflict:
1. Is this intended to be a general purpose API that might see a
multitude of different users, or is it thought of as an API designed
to support a few specific users such as D-Bus and maybe a handful of
others? I had thought the former, but when you point me in the
direction of the D-Bus spec, I start to have doubts.
2. Is the API to be invoked directly by applications or is intended to
be used only behind specific libraries? You seem to be saying that
the latter is the case (here, I'm referring to your comment above
about sd-bus). However, when I asked David Herrmann a similar
question I got this responser:
"kdbus is in no way bound to systemd. There are ongoing efforts
to port glib and qt to kdbus natively. The API is pretty simple
and I don't see how a libkdbus would simplify things. In fact,
even our tests only have slim wrappers around the ioctls to
simplify error-handling in test-scenarios."
To me, that implies that users will employ the raw kernel API.
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Michael Kerrisk (man-pages) @ 2015-01-26 14:46 UTC (permalink / raw)
To: Greg Kroah-Hartman, Austin S Hemmelgarn
Cc: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, David Herrmann, Daniel Mack,
Arnd Bergmann, Eric W. Biederman, One Thousand Gnomes,
Tom Gundersen, Theodore T'so, Andy Lutomirski, Linux API,
linux-kernel, Djalal Harouni, Johannes Stezenbach,
Christoph Hellwig
In-Reply-To: <20150123160854.GA5210-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
Hello Greg,
On 01/23/2015 05:08 PM, Greg Kroah-Hartman wrote:
> On Thu, Jan 22, 2015 at 09:49:00AM -0500, Austin S Hemmelgarn wrote:
>> While I agree that there should be a way for userspace to get the list of
>> supported operations, userspace apps will only actually care about that
>> once, when they begin talking to kdbus, because (ignoring the live kernel
>> patching that people have been working on recently) the list of supported
>> operations isn't going to change while the system is running. While a u64
>> copy has relatively low overhead, it does have overhead, and that is very
>> significant when you consider part of the reason some people want kdbus is
>> for the performance gain. Especially for those automotive applications that
>> have been mentioned which fire off thousands of messages during start-up,
>> every little bit of performance is significant.
>
> A single u64 in a structure is not going to be measurable at all,
> processors just copy memory too fast these days for 4 extra bytes to be
> noticable.
It depends on the definition of measurable, I suppose, but this statement
appears incorrect to me. In some cases (e.g., kdbus_msg_info) we're talking
about *two* u64 fields (kernel_gs, kernel_msg_flags) being used to pass back
sets of valid flags. That's 16 bytes, and it definitely makes a difference.
Simply running a loop that does a naive memcpy() in a tight user-space
loop (code below), I see the following for the execution of 1e9 loops:
Including the two extra u64 fields: 3.2 sec
Without the two extra u64 fields: 2.6 sec
On the same box, doing 1e9 calls to getppid() (i.e., pretty much the
simplest syscall, giving us a rough measure of the context switch) takes
68 seconds. In other words, the cost of copying those 16 bytes is about 1%
of the base context switch/syscall cost. I assume the costs of copying
those 16 bytes across the kernel-user-space boundary would not be cheaper,
but have not tested that. If my assumption is correct, then 1% seems a
significant figure to me in an API whose raison d'être is speed.
> So let's make this as easy as possible for userspace, making
> it simpler logic there, which is much more important than saving
> theoretical time in the kernel.
But this also missed the other part of the point. Copying these fields on
every operation, when in fact they are only needed once, clutters the API,
in my opinion. Good APIs are as simple as they can be to do their job.
Redundancy is an enemy of simplicity. Simplest would have been a one time
API that returns a structure containing all of the supported flags across
the API. Alternatively, the traditional EINVAL approach is well understood,
and suffices.
Thanks,
Michael
=========
#include <stdint.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
struct kdbus_msg_info {
uint64_t offset;
uint64_t msg_size;
uint64_t return_flags;
};
struct kdbus_cmd_send {
uint64_t size;
uint64_t flags;
#if FIELDS >= 1
uint64_t kernel_flags;
#endif
#if FIELDS >= 2
uint64_t kernel_msg_flags;
#endif
uint64_t return_flags;
uint64_t msg_address;
struct kdbus_msg_info reply;
//struct kdbus_item items[0];
} __attribute__((aligned(8)));
int
main(int argc, char *argv[])
{
long nloops, j;
struct kdbus_cmd_send src, dst;
memset(&dst, 0, sizeof(struct kdbus_cmd_send));
printf("struct size: %zd\n", sizeof(struct kdbus_cmd_send));
nloops = (argc > 1) ? atol(argv[1]) : 1000000000;
for (j = 0; j < nloops; j++) {
memcpy(&dst, &src, sizeof(struct kdbus_cmd_send));
}
exit(EXIT_SUCCESS);
}
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Tom Gundersen @ 2015-01-26 15:26 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Greg Kroah-Hartman, Daniel Mack, Arnd Bergmann, Eric W. Biederman,
One Thousand Gnomes, Jiri Kosina, Andy Lutomirski, Linux API,
LKML, David Herrmann, Djalal Harouni, Johannes Stezenbach,
Theodore T'so, christoph Hellwig
In-Reply-To: <54C65267.7090905@gmail.com>
Hi Michael,
On Mon, Jan 26, 2015 at 3:42 PM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> 2. Is the API to be invoked directly by applications or is intended to
> be used only behind specific libraries? You seem to be saying that
> the latter is the case (here, I'm referring to your comment above
> about sd-bus). However, when I asked David Herrmann a similar
> question I got this responser:
>
> "kdbus is in no way bound to systemd. There are ongoing efforts
> to port glib and qt to kdbus natively. The API is pretty simple
> and I don't see how a libkdbus would simplify things. In fact,
> even our tests only have slim wrappers around the ioctls to
> simplify error-handling in test-scenarios."
>
> To me, that implies that users will employ the raw kernel API.
The way I read this is that there will (probably) be a handful of
users, namely the existing dbus libraries: libdus, sd-bus, glib, Qt,
ell, and maybe a few others. However, third-party developers will not
know/care about the details of kdbus, they'll just be coding against
the dbus libraries as before (might be minor changes, but they
certainly won't need to know anything about the kernel API). Similarly
to how userspace developers now code against their libc of choice,
rather than use kernel syscalls directly.
HTH,
Tom
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: christoph Hellwig @ 2015-01-26 16:44 UTC (permalink / raw)
To: Tom Gundersen
Cc: Michael Kerrisk (man-pages), Greg Kroah-Hartman, Daniel Mack,
Arnd Bergmann, Eric W. Biederman, One Thousand Gnomes,
Jiri Kosina, Andy Lutomirski, Linux API, LKML, David Herrmann,
Djalal Harouni, Johannes Stezenbach, Theodore T'so,
christoph Hellwig
In-Reply-To: <CAG-2HqUCZjc4Ucc-L21uG6e-di4UcLJAT5mW_-5-c-uqmoJyzA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Jan 26, 2015 at 04:26:53PM +0100, Tom Gundersen wrote:
> The way I read this is that there will (probably) be a handful of
> users, namely the existing dbus libraries: libdus, sd-bus, glib, Qt,
> ell, and maybe a few others. However, third-party developers will not
> know/care about the details of kdbus, they'll just be coding against
> the dbus libraries as before (might be minor changes, but they
> certainly won't need to know anything about the kernel API). Similarly
> to how userspace developers now code against their libc of choice,
> rather than use kernel syscalls directly.
Which means we do need proper man pages and detailed documentation for
it, just like syscalls for syscalls which just happened to be used by
a few libcs. I suspect it really should be implemented as
syscalls anyway, but we can leave that argument aside from now. Good
documentation certainly helps with making that decision in an educated
way.
^ permalink raw reply
* Re: [PATCH 01/13] kdbus: add documentation
From: Michael Kerrisk (man-pages) @ 2015-01-26 16:45 UTC (permalink / raw)
To: Tom Gundersen
Cc: mtk.manpages, Greg Kroah-Hartman, Daniel Mack, Arnd Bergmann,
Eric W. Biederman, One Thousand Gnomes, Jiri Kosina,
Andy Lutomirski, Linux API, LKML, David Herrmann, Djalal Harouni,
Johannes Stezenbach, Theodore T'so, christoph Hellwig
In-Reply-To: <CAG-2HqUCZjc4Ucc-L21uG6e-di4UcLJAT5mW_-5-c-uqmoJyzA@mail.gmail.com>
On 01/26/2015 04:26 PM, Tom Gundersen wrote:
> Hi Michael,
>
> On Mon, Jan 26, 2015 at 3:42 PM, Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
>> 2. Is the API to be invoked directly by applications or is intended to
>> be used only behind specific libraries? You seem to be saying that
>> the latter is the case (here, I'm referring to your comment above
>> about sd-bus). However, when I asked David Herrmann a similar
>> question I got this responser:
>>
>> "kdbus is in no way bound to systemd. There are ongoing efforts
>> to port glib and qt to kdbus natively. The API is pretty simple
>> and I don't see how a libkdbus would simplify things. In fact,
>> even our tests only have slim wrappers around the ioctls to
>> simplify error-handling in test-scenarios."
>>
>> To me, that implies that users will employ the raw kernel API.
>
> The way I read this is that there will (probably) be a handful of
> users, namely the existing dbus libraries: libdus, sd-bus, glib, Qt,
> ell, and maybe a few others. However, third-party developers will not
> know/care about the details of kdbus, they'll just be coding against
> the dbus libraries as before (might be minor changes, but they
> certainly won't need to know anything about the kernel API). Similarly
> to how userspace developers now code against their libc of choice,
> rather than use kernel syscalls directly.
Thanks, Tom, for the input. I'm still confused though, since elsewhere
in this thread David Herrmann said in response to a question of mine:
I think we can agree that we want it to be generically useful,
like other ipc mechanisms, including UDS and netlink.
Again, that sounds to me like the vision is not "a handful of users".
Hopefully Greg and David can clarify.
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
From: Priit Laes @ 2015-01-26 16:58 UTC (permalink / raw)
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Priit Laes, Maxime Ripard,
Hans de Goede, Dmitry Torokhov, open list:ABI/API,
moderated list:ARM/Allwinner A1X..., open list,
open list:SUN4I LOW RES ADC...
---
.../ABI/testing/sysfs-driver-input-sun4i-lradc | 4 ++
drivers/input/keyboard/sun4i-lradc-keys.c | 49 +++++++++++++++++-----
2 files changed, 43 insertions(+), 10 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
new file mode 100644
index 0000000..e4e6448
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
@@ -0,0 +1,4 @@
+What: /sys/class/input/input(x)/device/voltage
+Date: February 2015
+Contact: Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
+Description: ADC output voltage in microvolts or 0 if device is not opened.
diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
index cc8f7dd..c0ab8ec 100644
--- a/drivers/input/keyboard/sun4i-lradc-keys.c
+++ b/drivers/input/keyboard/sun4i-lradc-keys.c
@@ -79,10 +79,27 @@ struct sun4i_lradc_data {
u32 vref;
};
+static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
+{
+ u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
+ return val * lradc->vref / 63;
+};
+
+static ssize_t
+sun4i_lradc_dev_voltage_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
+}
+
+static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
+
static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
{
struct sun4i_lradc_data *lradc = dev_id;
- u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
+ u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
ints = readl(lradc->base + LRADC_INTS);
@@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
}
if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
- val = readl(lradc->base + LRADC_DATA0) & 0x3f;
- voltage = val * lradc->vref / 63;
+ voltage = sun4i_lradc_read_voltage(lradc);
for (i = 0; i < lradc->chan0_map_count; i++) {
diff = abs(lradc->chan0_map[i].voltage - voltage);
@@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
}
static int sun4i_lradc_load_dt_keymap(struct device *dev,
- struct sun4i_lradc_data *lradc)
+ struct sun4i_lradc_data *lradc)
{
struct device_node *np, *pp;
int i;
@@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
lradc->chan0_map_count = of_get_child_count(np);
if (lradc->chan0_map_count == 0) {
- dev_err(dev, "keymap is missing in device tree\n");
- return -EINVAL;
+ dev_info(dev, "keymap is missing in device tree\n");
+ return 0;
}
lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
@@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
error = of_property_read_u32(pp, "channel", &channel);
if (error || channel != 0) {
- dev_err(dev, "%s: Inval channel prop\n", pp->name);
+ dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
return -EINVAL;
}
error = of_property_read_u32(pp, "voltage", &map->voltage);
if (error) {
- dev_err(dev, "%s: Inval voltage prop\n", pp->name);
+ dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
return -EINVAL;
}
error = of_property_read_u32(pp, "linux,code", &map->keycode);
if (error) {
- dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
+ dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
return -EINVAL;
}
@@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
if (error)
return error;
- error = input_register_device(lradc->input);
+ error = device_create_file(dev, &dev_attr_voltage);
if (error)
return error;
+ error = input_register_device(lradc->input);
+ if (error) {
+ device_remove_file(&pdev->dev, &dev_attr_voltage);
+ return error;
+ }
+
platform_set_drvdata(pdev, lradc);
return 0;
}
+static int sun4i_lradc_remove(struct platform_device *pdev)
+{
+ device_remove_file(&pdev->dev, &dev_attr_voltage);
+ return 0;
+}
+
static const struct of_device_id sun4i_lradc_of_match[] = {
{ .compatible = "allwinner,sun4i-a10-lradc-keys", },
{ /* sentinel */ }
@@ -277,6 +305,7 @@ static struct platform_driver sun4i_lradc_driver = {
.of_match_table = of_match_ptr(sun4i_lradc_of_match),
},
.probe = sun4i_lradc_probe,
+ .remove = sun4i_lradc_remove,
};
module_platform_driver(sun4i_lradc_driver);
--
2.2.2
^ permalink raw reply related
* Re: [PATCH] arm: sunxi: input: RFC: Add sysfs voltage for sun4i-lradc driver
From: Hans de Goede @ 2015-01-26 19:28 UTC (permalink / raw)
To: Priit Laes
Cc: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Maxime Ripard,
Dmitry Torokhov, ABI/API, moderated list:ARM/Allwinner A1X...,
open list, open list:SUN4I LOW RES ADC...
In-Reply-To: <1422291516-24895-1-git-send-email-plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
Hi,
On 26-01-15 17:58, Priit Laes wrote:
No commit message? Please write an informative commit msg, like why we want this patch,
I guess it is to help figuring out the voltage levels for various buttons when creating
a dts, but I would prefer to not guess, which is where a good commit message would
come in handy ...
> ---
> .../ABI/testing/sysfs-driver-input-sun4i-lradc | 4 ++
> drivers/input/keyboard/sun4i-lradc-keys.c | 49 +++++++++++++++++-----
> 2 files changed, 43 insertions(+), 10 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> new file mode 100644
> index 0000000..e4e6448
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-input-sun4i-lradc
> @@ -0,0 +1,4 @@
> +What: /sys/class/input/input(x)/device/voltage
> +Date: February 2015
> +Contact: Priit Laes <plaes-q/aMd4JkU83YtjvyW6yDsg@public.gmane.org>
> +Description: ADC output voltage in microvolts or 0 if device is not opened.
> diff --git a/drivers/input/keyboard/sun4i-lradc-keys.c b/drivers/input/keyboard/sun4i-lradc-keys.c
> index cc8f7dd..c0ab8ec 100644
> --- a/drivers/input/keyboard/sun4i-lradc-keys.c
> +++ b/drivers/input/keyboard/sun4i-lradc-keys.c
> @@ -79,10 +79,27 @@ struct sun4i_lradc_data {
> u32 vref;
> };
>
> +static u32 sun4i_lradc_read_voltage(struct sun4i_lradc_data *lradc)
> +{
> + u32 val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> + return val * lradc->vref / 63;
> +};
> +
> +static ssize_t
> +sun4i_lradc_dev_voltage_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct sun4i_lradc_data *lradc = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%u\n", sun4i_lradc_read_voltage(lradc));
> +}
> +
> +static const DEVICE_ATTR(voltage, S_IRUGO, sun4i_lradc_dev_voltage_show, NULL);
> +
> static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> {
> struct sun4i_lradc_data *lradc = dev_id;
> - u32 i, ints, val, voltage, diff, keycode = 0, closest = 0xffffffff;
> + u32 i, ints, voltage, diff, keycode = 0, closest = 0xffffffff;
>
> ints = readl(lradc->base + LRADC_INTS);
>
> @@ -97,8 +114,7 @@ static irqreturn_t sun4i_lradc_irq(int irq, void *dev_id)
> }
>
> if ((ints & CHAN0_KEYDOWN_IRQ) && lradc->chan0_keycode == 0) {
> - val = readl(lradc->base + LRADC_DATA0) & 0x3f;
> - voltage = val * lradc->vref / 63;
> + voltage = sun4i_lradc_read_voltage(lradc);
>
> for (i = 0; i < lradc->chan0_map_count; i++) {
> diff = abs(lradc->chan0_map[i].voltage - voltage);
> @@ -156,7 +172,7 @@ static void sun4i_lradc_close(struct input_dev *dev)
> }
>
> static int sun4i_lradc_load_dt_keymap(struct device *dev,
> - struct sun4i_lradc_data *lradc)
> + struct sun4i_lradc_data *lradc)
> {
> struct device_node *np, *pp;
> int i;
Why this identation change ?
> @@ -168,8 +184,8 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>
> lradc->chan0_map_count = of_get_child_count(np);
> if (lradc->chan0_map_count == 0) {
> - dev_err(dev, "keymap is missing in device tree\n");
> - return -EINVAL;
> + dev_info(dev, "keymap is missing in device tree\n");
> + return 0;
> }
>
> lradc->chan0_map = devm_kmalloc_array(dev, lradc->chan0_map_count,
I assume this is so that people can still use the sysfs node, to create a dts, right
not sure I like this, might be better to document to simple create a dts with
a single button mapping for 200 mV (most board use 200 mV steps between the buttons).
> @@ -185,19 +201,19 @@ static int sun4i_lradc_load_dt_keymap(struct device *dev,
>
> error = of_property_read_u32(pp, "channel", &channel);
> if (error || channel != 0) {
> - dev_err(dev, "%s: Inval channel prop\n", pp->name);
> + dev_err(dev, "%s: Invalid 'channel' property\n", pp->name);
> return -EINVAL;
> }
>
> error = of_property_read_u32(pp, "voltage", &map->voltage);
> if (error) {
> - dev_err(dev, "%s: Inval voltage prop\n", pp->name);
> + dev_err(dev, "%s: Invalid 'voltage' property\n", pp->name);
> return -EINVAL;
> }
>
> error = of_property_read_u32(pp, "linux,code", &map->keycode);
> if (error) {
> - dev_err(dev, "%s: Inval linux,code prop\n", pp->name);
> + dev_err(dev, "%s: Invalid 'linux,code' property\n", pp->name);
> return -EINVAL;
> }
>
This hunk / 3 changes belong in a separate patch. Also please run checkpatch, I think
you're running over 80 chars here.
> @@ -257,14 +273,26 @@ static int sun4i_lradc_probe(struct platform_device *pdev)
> if (error)
> return error;
>
> - error = input_register_device(lradc->input);
> + error = device_create_file(dev, &dev_attr_voltage);
> if (error)
> return error;
>
> + error = input_register_device(lradc->input);
> + if (error) {
> + device_remove_file(&pdev->dev, &dev_attr_voltage);
> + return error;
> + }
> +
> platform_set_drvdata(pdev, lradc);
> return 0;
> }
>
> +static int sun4i_lradc_remove(struct platform_device *pdev)
> +{
> + device_remove_file(&pdev->dev, &dev_attr_voltage);
> + return 0;
> +}
> +
This looks wrong, I think (*) that we've a bug here because we're not
unregistering the input device, so maybe do 2 patches, 1 fixing the
not unregistering bug, and then just add the device_remove_file()
in the sysfs patch.
> static const struct of_device_id sun4i_lradc_of_match[] = {
> { .compatible = "allwinner,sun4i-a10-lradc-keys", },
> { /* sentinel */ }
> @@ -277,6 +305,7 @@ static struct platform_driver sun4i_lradc_driver = {
> .of_match_table = of_match_ptr(sun4i_lradc_of_match),
> },
> .probe = sun4i_lradc_probe,
> + .remove = sun4i_lradc_remove,
> };
>
> module_platform_driver(sun4i_lradc_driver);
>
Regards,
Hans
*) But I'm not sure, better double check.
^ 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