* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Eric Dumazet @ 2015-05-04 15:12 UTC (permalink / raw)
To: John Heffner
Cc: Eric B Munson, Tom Herbert, David S. Miller,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev
In-Reply-To: <CABrhC0nmsfAyHgJX8zEBDBVfFN=2qXKy7cO0Kbp9R9UCeEYowg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 2015-05-04 at 10:41 -0400, John Heffner wrote:
> Nice idea, seems handy. But a couple (somewhat related) questions:
>
> * Other than convenience, are there reasons not use an existing, more
> general-purpose and portable mechanism like pcap? (Permissions, I
> guess?)
Very hard to synchronize when say you have 32 listeners sharing a single
port (SO_REUSEPORT), and receive one million SYN per second (when my TCP
listener scaling work is finished).
libpcap here would be a serious bottleneck, even with a clever FANIN
support on the af_packet sockets, considering use of multiqueue NIC.
> * Are there conditions where, for security purposes, you don't want an
> application to have access to the raw SYNs?
Not that we are aware of : We restrict the access to IP + TCP headers,
for the passive part. All information that is available there was
provided by the remote peer on a 'open way' anyway.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Eric B Munson @ 2015-05-04 14:58 UTC (permalink / raw)
To: John Heffner
Cc: Eric Dumazet, Tom Herbert, David S. Miller,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev
In-Reply-To: <CABrhC0nmsfAyHgJX8zEBDBVfFN=2qXKy7cO0Kbp9R9UCeEYowg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]
On Mon, 04 May 2015, John Heffner wrote:
> On Mon, May 4, 2015 at 12:34 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >
> > This patch allows a server application to get the TCP SYN headers for
> > its passive connections. This is useful if the server is doing
> > fingerprinting of clients based on SYN packet contents.
> >
> > Two socket options are added: TCP_SAVE_SYN and TCP_SAVED_SYN.
> >
> > The first is used on a socket to enable saving the SYN headers
> > for child connections. This can be set before or after the listen()
> > call.
> >
> > The latter is used to retrieve the SYN headers for passive connections,
> > if the parent listener has enabled TCP_SAVE_SYN.
> >
> > TCP_SAVED_SYN is read once, it frees the saved SYN headers.
> >
> > The data returned in TCP_SAVED_SYN are network (IPv4/IPv6) and TCP
> > headers.
> >
> > Original patch was written by Tom Herbert, I changed it to not hold
> > a full skb (and associated dst and conntracking reference).
> >
> > We have used such patch for about 3 years at Google.
>
>
> Nice idea, seems handy. But a couple (somewhat related) questions:
>
> * Other than convenience, are there reasons not use an existing, more
> general-purpose and portable mechanism like pcap? (Permissions, I
> guess?)
Eric Dumazet can speak for their reasons, but we had a difficult time
guaranteeing that the data collected by libpcap was avaialble to the
accepting thread when accept() returned without making the accepting
thread wait on that data. With this implementation, we guarantee that
userspace has access to this data when accept() returns.
> * Are there conditions where, for security purposes, you don't want an
> application to have access to the raw SYNs?
In this implementation, userspace does not have access to the raw SYN,
rather it can request a copy of the TCP and IP headers.
Eric
>
> Thanks,
> -John
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: John Heffner @ 2015-05-04 14:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric B Munson, Tom Herbert, David S. Miller,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev
In-Reply-To: <1430714086.3711.165.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Mon, May 4, 2015 at 12:34 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> This patch allows a server application to get the TCP SYN headers for
> its passive connections. This is useful if the server is doing
> fingerprinting of clients based on SYN packet contents.
>
> Two socket options are added: TCP_SAVE_SYN and TCP_SAVED_SYN.
>
> The first is used on a socket to enable saving the SYN headers
> for child connections. This can be set before or after the listen()
> call.
>
> The latter is used to retrieve the SYN headers for passive connections,
> if the parent listener has enabled TCP_SAVE_SYN.
>
> TCP_SAVED_SYN is read once, it frees the saved SYN headers.
>
> The data returned in TCP_SAVED_SYN are network (IPv4/IPv6) and TCP
> headers.
>
> Original patch was written by Tom Herbert, I changed it to not hold
> a full skb (and associated dst and conntracking reference).
>
> We have used such patch for about 3 years at Google.
Nice idea, seems handy. But a couple (somewhat related) questions:
* Other than convenience, are there reasons not use an existing, more
general-purpose and portable mechanism like pcap? (Permissions, I
guess?)
* Are there conditions where, for security purposes, you don't want an
application to have access to the raw SYNs?
Thanks,
-John
^ permalink raw reply
* Re: [RFC PATCH 2/3] iio: allow better control for flushing the hardware fifo
From: Lars-Peter Clausen @ 2015-05-04 14:38 UTC (permalink / raw)
To: Octavian Purdila
Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald,
linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, lkml,
Adriana Reus, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAE1zotKDQjQS70tY0NS6536pjmKW8mbH_p+2aOv0kM0075hgkA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 05/03/2015 08:11 AM, Octavian Purdila wrote:
> On Sat, May 2, 2015 at 8:42 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>> On 04/29/2015 01:18 PM, Octavian Purdila wrote:
>>>
>>> Some applications need to be able to flush [1] the hardware fifo of
>>> the device and to receive events of when that happened [2] so that it
>>> can ignore stale data.
>>>
>>> This patch adds a new event (IIO_EV_TYPE_HWFIFO_FLUSHED) that should
>>> be sent to userspace when a flush has been completed. The application
>>> will be able to identify which are the samples to ignore based on the
>>> timestamp of the event.
>>>
>>> To allow applications to accurately generate a hardware fifo flush on
>>> demand, this patch also adds a new sysfs entry that triggers a
>>> hardware fifo flush when written to.
>>>
>>> [1]
>>> https://source.android.com/devices/sensors/hal-interface.html#flush_sensor
>>> [2]
>>> https://source.android.com/devices/sensors/hal-interface.html#metadata_flush_complete_events
>>
>>
>> Since there is no asynchronous queue for commands to be executed in IIO
>> adding a asynchronous completion event doesn't make too much sense. This is
>> something that needs to be handled at the HAL level.
>>
>> The HAL needs to have a queue of commands that need to be executed where new
>> events can be added asynchronously, then has a loop which goes through the
>> commands in the queue and executes them, and once executed generated the
>> appropriate completion event.
>>
>
> Hi Lars,
>
> Thanks for the review.
>
> We can't do this at the HAL level because the needed information is
> only available at the HAL level. At the HAL level each received sample
> from the driver is converted to an event. When doing a flush the HAL
> must add a special event (flush complete) after the last sample in the
> hardware fifo. But the HAL does not know how many samples are in the
> hardware fifo, how many are in the device buffer, etc.
Ok, so that's what you need the timestamp for I presume? So the signature of
the of the sync function is basically.
timestamp sync(device)
where timestamp is greater or equal to the timestamp of all samples before
the sync and smaller or equal to all samples after the sync.
What your implementation does is implement a synchronous API to flush the
FIFO but delivers the result of the operation asynchronously via a rather
arbitrary delivery mechanism. That is in my opinion not a good API/ABI and
might even have some race condition issues.
If you do a flush, then read as much data as available you know that this
data is from before the flush and any data read at a later point is after
the flush.
>
>>
>> I really wish that document would specify what is actually meant by flush.
>> Copy the FIFO content to a software buffer or discard the FIFO content.
>>
>
> It does say: "... and flushes the FIFO; those events are delivered as
> usual (i.e.: as if the maximum reporting latency had expired) ..."
>
Missed that, thanks.
>>
>>>
>>> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>> ---
>>> Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
>>> include/linux/iio/sysfs.h | 3 +++
>>> include/uapi/linux/iio/types.h | 1 +
>>> 3 files changed, 15 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>>> b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 866b4ec..bb4d8de 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1375,3 +1375,14 @@ Description:
>>> The emissivity ratio of the surface in the field of view
>>> of the
>>> contactless temperature sensor. Emissivity varies from 0
>>> to 1,
>>> with 1 being the emissivity of a black body.
>>> +
>>> +What: /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo_flush
>>> +KernelVersion: 4.2
>>> +Contact: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> +Description:
>>> + Write only entry that accepts a single strictly positive
>>> integer
>>> + specifying the number of samples to flush from the
>>> hardware fifo
>>> + to the device buffer. When the flush is completed an
>>> + IIO_EV_TYPE_HWFIFO_FLUSHED event is generated. The event
>>> has the
>>> + timestamp equal with the timestamp of last sample that was
>>> + flushed from the hardware fifo.
>>
>>
>> I'd prefer this to be handled through the normal read() API rather than
>> having a side channel for it. Big question is how though. We could specify
>> that reading in O_NONBLOCK mode will always read data if it is available and
>> not only if it is above the watermark threshold.
>
> Do you mean to try and flush when the available data in the device
> buffer is less then the requested size? That should work and hopefully
> the ABI change does not matter since the hwfifo stuff has not been
> released yet.
Basically only let poll() and blocking read() block when not enough data is
available. But for non-blocking read return as much data as possible if data
is available.
>
> I prefer the explicit flush though. I think it is better to have the
> ABIs clearly visible instead of being buried in the details.
>
Yea, it's a bit tricky. What should be used for this sysfs, IOCTL, read()...
- Lars
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Eric Dumazet @ 2015-05-04 14:36 UTC (permalink / raw)
To: Eric B Munson
Cc: Tom Herbert, David S. Miller, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev
In-Reply-To: <1430749912.3711.173.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Mon, 2015-05-04 at 07:31 -0700, Eric Dumazet wrote:
> On Mon, 2015-05-04 at 10:21 -0400, Eric B Munson wrote:
>
> > The getsockopt() for TCP_SAVE_SYN doesn't offer much over returning 0
> > from TCP_SAVED_SYN if the headers are not available. Why not collapse
> > these into a single option? If you need to track the difference between
> > the headers are not avialable and should have been and the headers
> > should not be available it can be done through error codes from
> > getsockopt().
>
> A single option is not good, because it adds confusion.
>
> Note that we need to set the bit on any socket state, and doing this
> with a single option is not very easy. ( You got this wrong in your
> patch btw)
Another argument for having two options is for TCP_REPAIR support.
If you want to properly check/restore a socket, including its
TCP_SAVE_SYN state, then you definitely need 2 options.
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Eric Dumazet @ 2015-05-04 14:31 UTC (permalink / raw)
To: Eric B Munson
Cc: Tom Herbert, David S. Miller, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev
In-Reply-To: <20150504142155.GD6113-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org>
On Mon, 2015-05-04 at 10:21 -0400, Eric B Munson wrote:
> The getsockopt() for TCP_SAVE_SYN doesn't offer much over returning 0
> from TCP_SAVED_SYN if the headers are not available. Why not collapse
> these into a single option? If you need to track the difference between
> the headers are not avialable and should have been and the headers
> should not be available it can be done through error codes from
> getsockopt().
A single option is not good, because it adds confusion.
Note that we need to set the bit on any socket state, and doing this
with a single option is not very easy. ( You got this wrong in your
patch btw)
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Eric B Munson @ 2015-05-04 14:21 UTC (permalink / raw)
To: Eric Dumazet
Cc: Tom Herbert, David S. Miller, linux-api-u79uwXL29TY76Z2rM5mHXA,
netdev
In-Reply-To: <1430714086.3711.165.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 8264 bytes --]
On Sun, 03 May 2015, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> This patch allows a server application to get the TCP SYN headers for
> its passive connections. This is useful if the server is doing
> fingerprinting of clients based on SYN packet contents.
>
> Two socket options are added: TCP_SAVE_SYN and TCP_SAVED_SYN.
>
> The first is used on a socket to enable saving the SYN headers
> for child connections. This can be set before or after the listen()
> call.
>
> The latter is used to retrieve the SYN headers for passive connections,
> if the parent listener has enabled TCP_SAVE_SYN.
>
> TCP_SAVED_SYN is read once, it frees the saved SYN headers.
>
> The data returned in TCP_SAVED_SYN are network (IPv4/IPv6) and TCP
> headers.
>
> Original patch was written by Tom Herbert, I changed it to not hold
> a full skb (and associated dst and conntracking reference).
>
> We have used such patch for about 3 years at Google.
>
> Signed-off-by: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> include/linux/tcp.h | 8 ++++++++
> include/net/request_sock.h | 4 +++-
> include/uapi/linux/tcp.h | 2 ++
> net/ipv4/tcp.c | 35 +++++++++++++++++++++++++++++++++++
> net/ipv4/tcp_input.c | 18 ++++++++++++++++++
> net/ipv4/tcp_ipv4.c | 1 +
> net/ipv4/tcp_minisocks.c | 3 +++
> 7 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 3b2911502a8c..e6fb5df22db1 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -199,6 +199,7 @@ struct tcp_sock {
> syn_fastopen:1, /* SYN includes Fast Open option */
> syn_fastopen_exp:1,/* SYN includes Fast Open exp. option */
> syn_data_acked:1,/* data in SYN is acked by SYN-ACK */
> + save_syn:1, /* Save headers of SYN packet */
> is_cwnd_limited:1;/* forward progress limited by snd_cwnd? */
> u32 tlp_high_seq; /* snd_nxt at the time of TLP retransmit. */
>
> @@ -326,6 +327,7 @@ struct tcp_sock {
> * socket. Used to retransmit SYNACKs etc.
> */
> struct request_sock *fastopen_rsk;
> + u32 *saved_syn;
> };
>
> enum tsq_flags {
> @@ -393,4 +395,10 @@ static inline int fastopen_init_queue(struct sock *sk, int backlog)
> return 0;
> }
>
> +static inline void tcp_saved_syn_free(struct tcp_sock *tp)
> +{
> + kfree(tp->saved_syn);
> + tp->saved_syn = NULL;
> +}
> +
> #endif /* _LINUX_TCP_H */
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 9f4265ce8892..87935cad2f7b 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -64,6 +64,7 @@ struct request_sock {
> struct timer_list rsk_timer;
> const struct request_sock_ops *rsk_ops;
> struct sock *sk;
> + u32 *saved_syn;
> u32 secid;
> u32 peer_secid;
> };
> @@ -77,7 +78,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener)
> req->rsk_ops = ops;
> sock_hold(sk_listener);
> req->rsk_listener = sk_listener;
> -
> + req->saved_syn = NULL;
> /* Following is temporary. It is coupled with debugging
> * helpers in reqsk_put() & reqsk_free()
> */
> @@ -104,6 +105,7 @@ static inline void reqsk_free(struct request_sock *req)
> req->rsk_ops->destructor(req);
> if (req->rsk_listener)
> sock_put(req->rsk_listener);
> + kfree(req->saved_syn);
> kmem_cache_free(req->rsk_ops->slab, req);
> }
>
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index faa72f4fa547..51ebedba577f 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -113,6 +113,8 @@ enum {
> #define TCP_TIMESTAMP 24
> #define TCP_NOTSENT_LOWAT 25 /* limit number of unsent bytes in write queue */
> #define TCP_CC_INFO 26 /* Get Congestion Control (optional) info */
> +#define TCP_SAVE_SYN 27 /* Record SYN headers for new connections */
> +#define TCP_SAVED_SYN 28 /* Get SYN headers recorded for connection */
The getsockopt() for TCP_SAVE_SYN doesn't offer much over returning 0
from TCP_SAVED_SYN if the headers are not available. Why not collapse
these into a single option? If you need to track the difference between
the headers are not avialable and should have been and the headers
should not be available it can be done through error codes from
getsockopt().
>
> struct tcp_repair_opt {
> __u32 opt_code;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 46efa03d2b11..ecccfdc50d76 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2482,6 +2482,13 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> icsk->icsk_syn_retries = val;
> break;
>
> + case TCP_SAVE_SYN:
> + if (val < 0 || val > 1)
> + err = -EINVAL;
> + else
> + tp->save_syn = val;
> + break;
> +
> case TCP_LINGER2:
> if (val < 0)
> tp->linger2 = -1;
> @@ -2818,6 +2825,34 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
> case TCP_NOTSENT_LOWAT:
> val = tp->notsent_lowat;
> break;
> + case TCP_SAVE_SYN:
> + val = tp->save_syn;
> + break;
> + case TCP_SAVED_SYN: {
> + if (get_user(len, optlen))
> + return -EFAULT;
> +
> + lock_sock(sk);
> + if (tp->saved_syn) {
What happens when userspace uses a buffer that is too small to hold the
saved headers? They get a partial result back but they do not get a
chance to come back for the rest (or really have any way of knowing
there was more beyond missing some of the tcp_hdr struct).
> + len = min_t(unsigned int, tp->saved_syn[0], len);
> + if (put_user(len, optlen)) {
> + release_sock(sk);
> + return -EFAULT;
> + }
> + if (copy_to_user(optval, tp->saved_syn + 1, len)) {
> + release_sock(sk);
> + return -EFAULT;
> + }
> + tcp_saved_syn_free(tp);
> + release_sock(sk);
> + } else {
> + release_sock(sk);
> + len = 0;
> + if (put_user(len, optlen))
> + return -EFAULT;
> + }
> + return 0;
> + }
> default:
> return -ENOPROTOOPT;
> }
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 09bdc4abfcbb..df2ca615cd0c 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6060,6 +6060,23 @@ static bool tcp_syn_flood_action(struct sock *sk,
> return want_cookie;
> }
>
> +static void tcp_reqsk_record_syn(const struct sock *sk,
> + struct request_sock *req,
> + const struct sk_buff *skb)
> +{
> + if (tcp_sk(sk)->save_syn) {
> + u32 len = skb_network_header_len(skb) + tcp_hdrlen(skb);
> + u32 *copy;
> +
> + copy = kmalloc(len + sizeof(u32), GFP_ATOMIC);
> + if (copy) {
> + copy[0] = len;
> + memcpy(©[1], skb_network_header(skb), len);
> + req->saved_syn = copy;
> + }
> + }
> +}
> +
> int tcp_conn_request(struct request_sock_ops *rsk_ops,
> const struct tcp_request_sock_ops *af_ops,
> struct sock *sk, struct sk_buff *skb)
> @@ -6192,6 +6209,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> tcp_rsk(req)->tfo_listener = false;
> af_ops->queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
> }
> + tcp_reqsk_record_syn(sk, req, skb);
>
> return 0;
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index fc1c658ec6c1..91cb4768a860 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1802,6 +1802,7 @@ void tcp_v4_destroy_sock(struct sock *sk)
>
> /* If socket is aborted during connect operation */
> tcp_free_fastopen_req(tp);
> + tcp_saved_syn_free(tp);
>
> sk_sockets_allocated_dec(sk);
> sock_release_memcg(sk);
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index e5d7649136fc..ebe2ab2596ed 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -536,6 +536,9 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
> newtp->fastopen_rsk = NULL;
> newtp->syn_data_acked = 0;
>
> + newtp->saved_syn = req->saved_syn;
> + req->saved_syn = NULL;
> +
> TCP_INC_STATS_BH(sock_net(sk), TCP_MIB_PASSIVEOPENS);
> }
> return newsk;
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Neal Cardwell @ 2015-05-04 14:02 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric B Munson, Tom Herbert, David S. Miller,
linux-api-u79uwXL29TY76Z2rM5mHXA, netdev
In-Reply-To: <1430714086.3711.165.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Mon, May 4, 2015 at 12:34 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> This patch allows a server application to get the TCP SYN headers for
> its passive connections. This is useful if the server is doing
> fingerprinting of clients based on SYN packet contents.
>
> Two socket options are added: TCP_SAVE_SYN and TCP_SAVED_SYN.
>
> The first is used on a socket to enable saving the SYN headers
> for child connections. This can be set before or after the listen()
> call.
>
> The latter is used to retrieve the SYN headers for passive connections,
> if the parent listener has enabled TCP_SAVE_SYN.
>
> TCP_SAVED_SYN is read once, it frees the saved SYN headers.
>
> The data returned in TCP_SAVED_SYN are network (IPv4/IPv6) and TCP
> headers.
>
> Original patch was written by Tom Herbert, I changed it to not hold
> a full skb (and associated dst and conntracking reference).
>
> We have used such patch for about 3 years at Google.
>
> Signed-off-by: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
Acked-by: Neal Cardwell <ncardwell-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Tested-by: Neal Cardwell <ncardwell-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
The code looks good to me, and I re-ran the test I wrote (and which I
see Eric posted), and double-checked that it passes on net-next with
this patch applied.
Personally I like the socket option names from this patch.
TCP_SAVE_SYN means "Please save SYN headers for child sockets of this
listener". And TCP_SAVED_SYN means "Please give me the saved SYN for
this accepted child."
Thanks, Eric!
neal
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Eric Dumazet @ 2015-05-04 13:53 UTC (permalink / raw)
To: mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w
Cc: Eric B Munson, Tom Herbert, David S. Miller, Linux API, netdev
In-Reply-To: <CAKgNAkiUOkjsE96E1DN_zwJAjJGLWME7-XGnFDszic7p7C=g7g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 2015-05-04 at 08:47 +0200, Michael Kerrisk (man-pages) wrote:
> Eric,
>
> On 4 May 2015 at 06:34, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> >
> > This patch allows a server application to get the TCP SYN headers for
> > its passive connections. This is useful if the server is doing
> > fingerprinting of clients based on SYN packet contents.
> >
> > Two socket options are added: TCP_SAVE_SYN and TCP_SAVED_SYN.
> >
> > The first is used on a socket to enable saving the SYN headers
> > for child connections. This can be set before or after the listen()
> > call.
> >
> > The latter is used to retrieve the SYN headers for passive connections,
> > if the parent listener has enabled TCP_SAVE_SYN.
> >
> > TCP_SAVED_SYN is read once, it frees the saved SYN headers.
> >
> > The data returned in TCP_SAVED_SYN are network (IPv4/IPv6) and TCP
> > headers.
>
> This description is a little thin, so I'm unclear on one or two
> points. TCP_SAVE_SYN is clearly applied to the listening socket. But
> what about TCP_SAVED_SYN? Is that applied to the connected socket
> returned by accept()?
>
> The highly similar naming of these two seems unfortunate. At the very
> least, it makes for easy confusion in conversations about the two
> options. It would be better to have names that were more distinct.
> Perhaps the latter could be TCP_CONN_SYN or TCP_CONN_SAVED_SYN, for
> example?
>
> Thanks,
TCP_CONN_SYN is rather confusing, because of the analogy with connect().
Maybe I can send the test Neal wrote 3 years ago, part of our automated
non regression tests we run here.
Let me know if you need more information, thanks !
/*
* Copyright 2012 Google Inc. All Rights Reserved.
* Author: ncardwell-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org (Neal Cardwell)
*
* A basic test for the TCP_SAVE_SYN and TCP_SAVED_SYN socket options.
*/
#include <assert.h>
#include <errno.h>
#include <netinet/in.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <unistd.h>
#ifndef TCP_SAVE_SYN
#define TCP_SAVE_SYN 27
#endif
#ifndef TCP_SAVED_SYN
#define TCP_SAVED_SYN 28
#endif
typedef char bool;
typedef enum bool_t {
false = 0,
true = 1,
} bool_t;
static void fail(const char *msg)
{
fprintf(stderr, "%s\n", msg);
exit(1);
}
static void fail_perror(const char *msg)
{
perror(msg);
exit(1);
}
/*
* Once every ~5000 connections, connect fails with ENOBUFS. This is
* not even in the manpage, and seem to be transient. For now, just retry.
*/
static void connect_reliably(int fd, struct sockaddr *daddr, int dlen)
{
int ret, max_runs = 5;
do {
ret = connect(fd, daddr, dlen);
} while (ret == -1 && errno == ENOBUFS && --max_runs);
if (ret)
fail_perror("reliable connect");
}
/* Get and validate the saved SYN. */
static void read_saved_syn(int fd, int address_family)
{
unsigned char syn[500];
socklen_t syn_len = sizeof(syn);
memset(syn, 0, sizeof(syn));
/* Read the saved SYN. */
if (getsockopt(fd, IPPROTO_TCP, TCP_SAVED_SYN, syn, &syn_len) != 0)
fail_perror("first getsockopt TCP_SAVED_SYN failed");
/* Check the length and first byte of the SYN. */
if (address_family == AF_INET) {
assert(syn_len == 60);
assert(syn[0] >> 4 == 0x4); /* IPv4 */
} else if (address_family == AF_INET6) {
assert(syn_len == 80);
assert(syn[0] >> 4 == 0x6); /* IPv6 */
} else {
assert(!"bad address family");
}
/* Check the last few bytes of the SYN, which will be TCP options. */
assert(syn[syn_len-4] == 0x01); /* TCP option: kind = NOP */
assert(syn[syn_len-3] == 0x03); /* TCP option: kind = window scale */
assert(syn[syn_len-2] == 0x03); /* TCP option: length = 3 */
assert(syn[syn_len-1] == 0x06 || syn[syn_len-1] == 0x07); /* TCP option: window scale = 6 or 7 */
/* If we try TCP_SAVED_SYN again it should succeed with 0 length. */
if (getsockopt(fd, IPPROTO_TCP, TCP_SAVED_SYN, syn, &syn_len) != 0)
fail("repeated getsockopt TCP_SAVED_SYN failed");
assert(syn_len == 0);
}
/* Open server and client socket and test TCP_SAVE_SYN and TCP_SAVED_SYN. */
static void do_test(struct sockaddr *srv_addr, uint16_t *srv_port, int srv_len,
struct sockaddr *cli_addr, uint16_t *cli_port, int cli_len,
bool get_saved_syn)
{
int fd_listen = -1, fd_accept = -1, fd_connect = -1;
int one = 1;
fd_listen = socket(srv_addr->sa_family, SOCK_STREAM, 0);
if (fd_listen == -1)
fail_perror("open fd_listen");
if (setsockopt(fd_listen, SOL_SOCKET, SO_REUSEADDR,
&one, sizeof(one)) < 0)
fail_perror("setsockopt SO_REUSEADDR");
if (bind(fd_listen, srv_addr, srv_len))
fail_perror("bind fd_listen");
if (getsockname(fd_listen, srv_addr, (socklen_t *)&srv_len))
fail_perror("getsockname fd_listen");
*cli_port = *srv_port;
if (setsockopt(fd_listen, IPPROTO_TCP, TCP_SAVE_SYN,
&one, sizeof(one)) < 0)
fail_perror("setsockopt TCP_SAVE_SYN");
if (listen(fd_listen, 1))
fail_perror("listen fd_listen");
fd_connect = socket(cli_addr->sa_family, SOCK_STREAM, 0);
if (fd_connect == -1)
fail_perror("open fd_connect");
connect_reliably(fd_connect, cli_addr, cli_len);
fd_accept = accept(fd_listen, NULL, 0);
if (fd_accept == -1)
fail_perror("accept fd_listen");
if (get_saved_syn) {
read_saved_syn(fd_accept, cli_addr->sa_family);
}
if (close(fd_listen))
fail_perror("close fd_listen");
if (close(fd_accept))
fail_perror("close fd_accept");
if (close(fd_connect))
fail_perror("close fd_connect");
}
static void test_ipv4(bool get_saved_syn)
{
struct sockaddr_in srv_addr = {
.sin_family = AF_INET,
.sin_addr.s_addr = INADDR_ANY,
.sin_port = 0,
};
struct sockaddr_in cli_addr = {
.sin_family = AF_INET,
.sin_addr.s_addr = htonl(INADDR_LOOPBACK),
};
printf(" testing IPv4 ...\n");
do_test((struct sockaddr *)&srv_addr, &srv_addr.sin_port,
sizeof(srv_addr),
(struct sockaddr *)&cli_addr, &cli_addr.sin_port,
sizeof(cli_addr),
get_saved_syn);
}
static void test_ipv6(bool get_saved_syn)
{
struct sockaddr_in6 srv_addr = {
.sin6_family = AF_INET6,
.sin6_addr = IN6ADDR_ANY_INIT,
.sin6_port = 0,
};
struct sockaddr_in6 cli_addr = {
.sin6_family = AF_INET6,
.sin6_addr = IN6ADDR_LOOPBACK_INIT,
};
printf(" testing IPv6 ...\n");
do_test((struct sockaddr *)&srv_addr, &srv_addr.sin6_port,
sizeof(srv_addr),
(struct sockaddr *)&cli_addr, &cli_addr.sin6_port,
sizeof(cli_addr),
get_saved_syn);
}
static void test_ipv4_mapped_ipv6(bool get_saved_syn)
{
struct sockaddr_in6 srv_addr = {
.sin6_family = AF_INET6,
.sin6_addr = IN6ADDR_ANY_INIT,
.sin6_port = 0,
};
struct sockaddr_in cli_addr = {
.sin_family = AF_INET,
.sin_addr.s_addr = htonl(INADDR_LOOPBACK),
};
printf(" testing IPv4-mapped-IPv6 (srv=AF_INET6, cli=AF_INET)...\n");
do_test((struct sockaddr *)&srv_addr, &srv_addr.sin6_port,
sizeof(srv_addr),
(struct sockaddr *)&cli_addr, &cli_addr.sin_port,
sizeof(cli_addr),
get_saved_syn);
}
static void test_all_address_families(bool get_saved_syn)
{
test_ipv4(get_saved_syn);
test_ipv6(get_saved_syn);
test_ipv4_mapped_ipv6(get_saved_syn);
}
static void run_all_tests(void)
{
bool get_saved_syn;
/* Test normal behavior, when we ask the kernel to record the
* SYN and then read it using the TCP_SAVED_SYN getsockopt().
*/
printf("test: reading the saved SYN...\n");
get_saved_syn = true;
test_all_address_families(get_saved_syn);
/* Test behavior when we ask the kernel to record the SYN and
* then never actually use the TCP_SAVED_SYN getsockopt() to
* extract the saved SYN.
*/
printf("test: not reading the saved SYN...\n");
get_saved_syn = false;
test_all_address_families(get_saved_syn);
}
static int usage(const char *executable_path)
{
fprintf(stderr, "usage: %s\n", executable_path);
return 1;
}
int main(int argc, char **argv)
{
if (getuid() != 0 || getgid() != 0)
fail("must run as root\n");
if (argc != 1)
return usage(argv[0]);
system("sysctl net.ipv4.tcp_timestamps=1");
system("sysctl net.ipv4.tcp_sack=1");
system("sysctl net.ipv4.tcp_window_scaling=1");
run_all_tests();
printf("OK. All tests passed.\n");
return 0;
}
^ permalink raw reply
* Re: [PATCH v4 01/10] regmap: Introduce regmap_get_max_register.
From: Mark Brown @ 2015-05-04 12:05 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: linux-arm-kernel, Maxime Ripard, Rob Herring, Kumar Gala, s.hauer,
Greg Kroah-Hartman, linux-api, linux-kernel, devicetree,
linux-arm-msm, arnd, sboyd
In-Reply-To: <1427752587-17087-1-git-send-email-srinivas.kandagatla@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 275 bytes --]
On Mon, Mar 30, 2015 at 10:56:27PM +0100, Srinivas Kandagatla wrote:
> +int regmap_get_max_register(struct regmap *map)
> +{
> + return map->max_register ? : -EINVAL;
> +}
Please don't abuse the ternery operator like this, write a proper if
statement that people can read.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects
From: Jonathan Cameron @ 2015-05-04 11:31 UTC (permalink / raw)
To: Octavian Purdila
Cc: lars, pmeerw, robert.moore, rafael.j.wysocki, lenb, linux-api,
linux-kernel, linux-iio, linux-acpi, Torokhov,
linux-input@vger.kernel.org
In-Reply-To: <1430146908-27919-4-git-send-email-octavian.purdila@intel.com>
On 27/04/15 16:01, Octavian Purdila wrote:
> This patch derives the mounting matrix for a particular IIO device
> based ont the ACPI _PLD information. Note that if mounting matrix is
> defined in the device properties it overrieds the _PLD information.
>
> Signed-off-by: Octavian Purdila <octavian.purdila@intel.com>
I have no objection to the functionality, but I'd prefer to separate it more
from the core of IIO. By all means have some utility functions to help with
what may be a common feature in future, but I don't want to have the data
stored in the main IIO structures. It's not relevant to lots of our devices
afterall!
Reading this (still offline unfortunately), looks like the ACPI provided data
is very very minimalist. Parts have to be on the panel and oriented at 90
degree increments? I suppose it covers a fair range of common hardware devices,
but far from all of them.
I'm not considerably more keen on the IIO interface as a substantial superset
of what ACPI is providing. Note that we should definitely discuss with
input (joysticks can have mounting matrices too!)
cc'd. Sorry Dmitry, think I managed to pick up an ancient version of your
email address for an earlier cc. Was being lazy and didn't check in MAINTAINERS.
Jonathan
> ---
> drivers/iio/industrialio-core.c | 51 +++++++++++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 46 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 97 insertions(+)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 9000c53..90ee58a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -31,6 +31,7 @@
> #include <linux/iio/sysfs.h>
> #include <linux/iio/events.h>
> #include <linux/iio/buffer.h>
> +#include <linux/acpi.h>
>
> /* IDA to assign each registered device a unique id */
> static DEFINE_IDA(iio_ida);
> @@ -871,6 +872,53 @@ static ssize_t iio_show_dev_name(struct device *dev,
>
> static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
>
> +#if IS_ENABLED(CONFIG_ACPI)
> +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
> +{
> + acpi_handle h = ACPI_HANDLE(indio_dev->dev.parent);
> + struct acpi_pld_info *info;
> +
> + if (ACPI_SUCCESS(acpi_get_physical_device_location(h, &info))) {
> + IIO_MM_SET(indio_dev->mounting_matrix, X, X, 1, 0);
> + IIO_MM_SET(indio_dev->mounting_matrix, Y, Y, 1, 0);
> + IIO_MM_SET(indio_dev->mounting_matrix, Z, Z, 1, 0);
> +
> + /* Chip placed on the back panel ; negate x and z */
> + if (info->panel == ACPI_PLD_PANEL_BACK) {
> + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> + IIO_MM_MUL(indio_dev->mounting_matrix, Z, Z, -1, 0);
> + }
> +
> + switch (info->rotation) {
> + case 2:
> + /* 90 deg clockwise: negate y then swap x,y */
> + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
> + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
> + break;
> + case 4:
> + /* Upside down: negate x and y */
> + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> + IIO_MM_MUL(indio_dev->mounting_matrix, Y, Y, -1, 0);
> + break;
> + case 6:
> + /* 90 deg counter clockwise: negate x then swap x,y */
> + IIO_MM_MUL(indio_dev->mounting_matrix, X, X, -1, 0);
> + IIO_MM_SWAP(indio_dev->mounting_matrix, X, Y);
> + break;
> + }
> +
> + return true;
> + }
> +
> + return false;
> +}
> +#else
> +static bool iio_get_mounting_matrix_acpi(struct iio_dev *indio_dev)
> +{
> + return false;
> +}
> +#endif
> +
> static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
> {
> int i, err;
> @@ -888,6 +936,9 @@ static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
> return true;
> }
>
> + if (ACPI_HANDLE(indio_dev->dev.parent))
> + return iio_get_mounting_matrix_acpi(indio_dev);
> +
> return false;
> }
>
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index c1fa852..feb7813 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -436,6 +436,52 @@ struct iio_buffer_setup_ops {
>
> #define IIO_MM_SIZE 18
>
> +enum {
> + IIO_MM_XX0,
> + IIO_MM_XX1,
> + IIO_MM_XY0,
> + IIO_MM_XY1,
> + IIO_MM_XZ0,
> + IIO_MM_XZ1,
> + IIO_MM_YX0,
> + IIO_MM_YX1,
> + IIO_MM_YY0,
> + IIO_MM_YY1,
> + IIO_MM_YZ0,
> + IIO_MM_YZ1,
> + IIO_MM_ZX0,
> + IIO_MM_ZX1,
> + IIO_MM_ZY0,
> + IIO_MM_ZY1,
> + IIO_MM_ZZ0,
> + IIO_MM_ZZ1,
> +};
> +
> +#define IIO_MM_SET(mm, a, b, val0, val1) \
> + do { \
> + mm[IIO_MM_##a##b##0] = val0; \
> + mm[IIO_MM_##a##b##1] = val1; \
> + } while (0) \
> +
> +#define IIO_MM_MUL(mm, a, b, val0, val1) \
> + do { \
> + mm[IIO_MM_##a##b##0] *= val0; \
> + mm[IIO_MM_##a##b##0] *= val1; \
> + } while (0) \
> +
> +#define IIO_MM_SWAP(mm, x, y) \
> + do { \
> + int tmp; \
> + \
> + tmp = mm[IIO_MM_##x##x##0]; \
> + mm[IIO_MM_##x##x##0] = mm[IIO_MM_##y##y##0]; \
> + mm[IIO_MM_##y##y##0] = tmp; \
> + \
> + tmp = mm[IIO_MM_##x##x##1]; \
> + mm[IIO_MM_##x##x##1] = mm[IIO_MM_##y##y##1]; \
> + mm[IIO_MM_##y##y##1] = tmp; \
> + } while (0) \
> +
> /**
> * struct iio_dev - industrial I/O device
> * @id: [INTERN] used to identify device internally
>
^ permalink raw reply
* Re: [PATCH v7 05/15] dt-bindings: Document the STM32 reset bindings
From: Maxime Coquelin @ 2015-05-04 11:25 UTC (permalink / raw)
To: Daniel Thompson, Philipp Zabel
Cc: Uwe Kleine-König, Andreas Färber, Geert Uytterhoeven,
Rob Herring, Linus Walleij, Arnd Bergmann, Stefan Agner,
Peter Meerwald, Paul Bolle, Peter Hurley, Andy Shevchenko,
Chanwoo Choi, Russell King, Daniel Lezcano, Joe Perches,
Vladimir Zapolskiy, Jonathan Corbet, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Thomas Gleixner
In-Reply-To: <5544A069.5000808@linaro.org>
2015-05-02 12:01 GMT+02:00 Daniel Thompson <daniel.thompson@linaro.org>:
> On 02/05/15 08:55, Maxime Coquelin wrote:
>>
>> 2015-05-01 10:08 GMT+02:00 Daniel Thompson <daniel.thompson@linaro.org>:
>>>
>>> On 30/04/15 17:20, Maxime Coquelin wrote:
>>>>
>>>>
>>>> This adds documentation of device tree bindings for the
>>>> STM32 reset controller.
>>>>
>>>> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>>>> ---
>>>> .../devicetree/bindings/reset/st,stm32-rcc.txt | 107
>>>> +++++++++++++++++++++
>>>> 1 file changed, 107 insertions(+)
>>>> create mode 100644
>>>> Documentation/devicetree/bindings/reset/st,stm32-rcc.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/st,stm32-rcc.txt
>>>> b/Documentation/devicetree/bindings/reset/st,stm32-rcc.txt
>>>> new file mode 100644
>>>> index 0000000..c1b0f8d
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/reset/st,stm32-rcc.txt
>>>> @@ -0,0 +1,107 @@
>>>> +STMicroelectronics STM32 Peripheral Reset Controller
>>>> +====================================================
>>>> +
>>>> +The RCC IP is both a reset and a clock controller. This documentation
>>>> only
>>>> +documents the reset part.
>>>> +
>>>> +Please also refer to reset.txt in this directory for common reset
>>>> +controller binding usage.
>>>> +
>>>> +Required properties:
>>>> +- compatible: Should be "st,stm32-rcc"
>>>> +- reg: should be register base and length as documented in the
>>>> + datasheet
>>>> +- #reset-cells: 1, see below
>>>> +
>>>> +example:
>>>> +
>>>> +rcc: reset@40023800 {
>>>> + #reset-cells = <1>;
>>>> + compatible = "st,stm32-rcc";
>>>
>>>
>>>
>>> Do you intend the clock driver to use the same compatible string (given
>>> it
>>> is the same bit of hardware).
>>>
>>> If so, is it better to use st,stm32f4-rcc here? It seems unlikey to me
>>> that
>>> the register layout of the PLLs and dividers can be the same on the f7
>>> parts
>>> (and later).
>>
>>
>> I agree we need a compatible dedicate to f4 series for clocks, and
>> maybe even one for f429 (to be checked).
>> For the reset part, we don't have this need.
>>
>> So either we use only "st,stm32f4" as you suggest, or we can have both
>> in device tree:
>>
>> rcc: reset@40023800 {
>> #reset-cells = <1>;
>> compatible = "st,stm32f4-rcc", "st,stm32-rcc";
>> reg = <0x40023800 0x400>;
>> };
>>
>> What do you think?
>
>
> Having both makes sense. The reset driver probably doesn't care about
> differences between F4 and F7 (I know very little about F7 but I can't think
> of any obvious h/ware evolution that would confuse the current reset
> driver).
>
>
>>>> + reg = <0x40023800 0x400>;
>>>> +};
>>>> +
>>>> +Specifying softreset control of devices
>>>> +=======================================
>>>> +
>>>> +Device nodes should specify the reset channel required in their
>>>> "resets"
>>>> +property, containing a phandle to the reset device node and an index
>>>> specifying
>>>> +which channel to use.
>>>> +The index is the bit number within the RCC registers bank, starting
>>>> from
>>>> RCC
>>>> +base address.
>>>> +It is calculated as: index = register_offset / 4 * 32 + bit_offset.
>>>> +Where bit_offset is the bit offset within the register.
>>>> +For example, for CRC reset:
>>>> + crc = AHB1RSTR_offset / 4 * 32 + CRCRST_bit_offset = 0x10 / 4 * 32 +
>>>> 12
>>>> = 140
>>>> +
>>>> +example:
>>>> +
>>>> + timer2 {
>>>> + resets = <&rcc 256>;
>>>> + };
>>>> +
>>>> +List of valid indices for STM32F429:
>>>> + - gpioa: 128
>>>> + - gpiob: 129
>>>> ...
>>>> <snip>
>>>> ...
>>>> + - sai1: 310
>>>> + - ltdc: 314
>>>
>>>
>>>
>>> These numbers are stable for all STM32F4 family parts. Should this table
>>> go
>>> into a dt-bindings header file?
>>>
>>
>> This has already been discussed with Philipp and Arnd in earlier
>> versions of this series [0].
>> I initially created a header file, and we decided going this way finally.
>
>
> Thanks for the link. I had overlooked that (I only really started paying
> attention at v5; I should probably have looked further back before
> commenting).
>
> However...
>
> Arnd's concerns about mergability of headers can also be met by using h/ware
> values in the header file can't there. To be honest my comment was pretty
> heavily influenced after having read a recent patch from Rob Herring (
> https://lkml.org/lkml/2015/5/1/14 ) which does exactly this.
>
> The main reason I got interested in having a header is that the reset bits
> and the clock gate bits are encoded using the same bit patterns so I
> wondering it we could express that only once.
Ok, I understand your need, and it makes sense.
The problem is that the values defined today cannot be re-used
directly for clocks.
Since the calculation is starting from rcc base, there is a 128 offset.
To re-use the same values, maybe we should create a mfd dt-binding header file.
It would contain the bits definition starting from 0, and define
macros for both reset and clocks to add the offsets.
For example, includes/dt-bindings/mfd/stm32f4-rcc.h would look like:
#define GPIOA 0
#define GPIOB 1
...
#define LTDC 186
#define STM32F4_RESET(x) (x + 128)
#define STM32F4_CLOCK(x) (x + 384)
Then, in DT, a reset would be described like this:
timer2 {
resets = <&rcc STM32F4_RESET(TIM2)>;
};
Phillip, Daniel, does that look acceptable to you?
Kind regards,
Maxime
>
> I guess it doesn't matter that much, especially given there is only one
> .dtsi file, and we can add a header later and remain binary compatible.
> However if the same number set does end up repeated in different .dtsi files
> I think that would motivate adding a header for F4 family.
>
>
> Daniel.
>
^ permalink raw reply
* Re: [RFC PATCH 1/3] iio: export mounting matrix information
From: Jonathan Cameron @ 2015-05-04 11:25 UTC (permalink / raw)
To: Octavian Purdila
Cc: lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
robert.moore-ral2JQCrhuEAvxtiuMwx3w,
rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w,
lenb-DgEjT+Ai2ygdnm+yROfE0A, linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1430146908-27919-2-git-send-email-octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On 27/04/15 16:01, Octavian Purdila wrote:
> Export a new sysfs attribute that describes the placement of the
> sensor on the device in the form of a mounting matrix so that
> userspace can do the required correction to get accurate data:
>
> corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
> corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
> corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
>
> This information is read from the device properties of the parent of
> the IIO device.
>
> Signed-off-by: Octavian Purdila <octavian.purdila-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Hmm. This could be done with existing code and a new info_mask element.
The disadvantage is you'd have to change the read_raw to read_raw_multi
callback, but that's hardly a big job. If I could be bothered, I'd
move the whole subsystem over and drop read_raw entirely.
It was introduced precisely for this sort of element.
As I mentioned deeper in the thread. If we are going to introduce a mounting
matrix, I'd prefer a version that includes translation. It might not matter
to many devices but it has mattered a lot to me in the past and doesn't hurt
others that much (I used to write papers on how to estimate this stuff
from black box devices).
> ---
> Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++
> drivers/iio/industrialio-core.c | 42 +++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 9 +++++++
> 3 files changed, 62 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 866b4ec..d36476e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1375,3 +1375,14 @@ Description:
> The emissivity ratio of the surface in the field of view of the
> contactless temperature sensor. Emissivity varies from 0 to 1,
> with 1 being the emissivity of a black body.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/mounting_matrix
> +KernelVersion: 4.2
> +Contact: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:
> + A list of 9 floating point values that describes the
> + transformation needed to account for the way the sensor has been
> + placed on the PCB:
> + corrected X = MM[0] * X + MM[1] Y + MM[2] * Z
> + corrected Y = MM[3] * X + MM[4] Y + MM[5] * Z
> + corrected Z = MM[6] * X + MM[7] Y + MM[8] * Z
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 7c98bc1..9000c53 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -871,6 +871,45 @@ static ssize_t iio_show_dev_name(struct device *dev,
>
> static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
>
> +static bool iio_get_mounting_matrix(struct iio_dev *indio_dev)
> +{
> + int i, err;
> +
> + err = device_property_read_u32_array(&indio_dev->dev, "mounting-matrix",
> + indio_dev->mounting_matrix, 9);
> + if (!err) {
> + int *mm = indio_dev->mounting_matrix;
> +
> + for (i = IIO_MM_SIZE - 2; i > 0; i -= 2) {
> + mm[i] = mm[i / 2];
> + mm[i + 1] = 0;
> + }
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static ssize_t iio_show_mounting_matrix(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + char *tmp = buf;
> + int i;
> +
> + for (i = 0; i < IIO_MM_SIZE; i += 2) {
> + tmp += iio_format_value(tmp, IIO_VAL_INT_PLUS_MICRO, 2,
> + &indio_dev->mounting_matrix[i]);
> + if (((i + 2) / 2) % 3)
Hmm. if (i != 8) should also do the job with slightly less feedling of
magic..
> + tmp[-1] = ' ';
> + }
> + return tmp - buf;
> +}
> +
> +static DEVICE_ATTR(mounting_matrix, S_IRUGO, iio_show_mounting_matrix, NULL);
> +
> static int iio_device_register_sysfs(struct iio_dev *indio_dev)
> {
> int i, ret = 0, attrcount, attrn, attrcount_orig = 0;
> @@ -920,6 +959,9 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
> indio_dev->chan_attr_group.attrs[attrn++] = &p->dev_attr.attr;
> if (indio_dev->name)
> indio_dev->chan_attr_group.attrs[attrn++] = &dev_attr_name.attr;
> + if (iio_get_mounting_matrix(indio_dev))
> + indio_dev->chan_attr_group.attrs[attrn++] =
> + &dev_attr_mounting_matrix.attr;
>
> indio_dev->groups[indio_dev->groupcounter++] =
> &indio_dev->chan_attr_group;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index b1e46ae..c1fa852 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -434,6 +434,8 @@ struct iio_buffer_setup_ops {
> const unsigned long *scan_mask);
> };
>
> +#define IIO_MM_SIZE 18
> +
> /**
> * struct iio_dev - industrial I/O device
> * @id: [INTERN] used to identify device internally
> @@ -469,6 +471,12 @@ struct iio_buffer_setup_ops {
> * @groups: [INTERN] attribute groups
> * @groupcounter: [INTERN] index of next attribute group
> * @flags: [INTERN] file ops related flags including busy flag.
Hmm. No absolute need to have this new data in the iio_dev. I'd do it
via the read_raw_multi callback, though this will require converting read_raw
over to that more flexible version.
> + * @mounting_matrix: [INTERN] the mounting matrix is a series of 9
> + * IIO_VAL_INT_PLUS_MICRO pairs that describes the
> + * transformation needed to account for the way the sensor
> + * has been placed on the PCB. See the mounting_matrix
> + * entry in Documentation/ABI/testing/sysfs-bus-iio about
> + * details of the transformation operations.
> * @debugfs_dentry: [INTERN] device specific debugfs dentry.
> * @cached_reg_addr: [INTERN] cached register address for debugfs reads.
> */
> @@ -509,6 +517,7 @@ struct iio_dev {
> int groupcounter;
>
> unsigned long flags;
> + int mounting_matrix[IIO_MM_SIZE];
> #if defined(CONFIG_DEBUG_FS)
> struct dentry *debugfs_dentry;
> unsigned cached_reg_addr;
>
^ permalink raw reply
* Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects
From: Jonathan Cameron @ 2015-05-04 11:23 UTC (permalink / raw)
To: Octavian Purdila, sathyanarayanan.kuppuswamy
Cc: Lars-Peter Clausen, Peter Meerwald, Robert Moore,
Rafael J Wysocki, lenb, linux-api, lkml,
linux-iio@vger.kernel.org, linux-acpi@vger.kernel.org,
Srinivas Pandruvada, Dmitry Torokhov, linux-input@vger.kernel.org
In-Reply-To: <CAE1zot+fusrvow+s2+CmoctcSD=c2BxoSyd2b=7MBv-pg68A+Q@mail.gmail.com>
On 28/04/15 03:23, Octavian Purdila wrote:
> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Hi
>>
>> On 04/27/2015 08:54 AM, Octavian Purdila wrote:
>>>
>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>
>>>> Since Acpi framework already exports this info to user space, Why not do
>>>> this derivation in user space code ? Why do we need new ABI, if the same
>>>> can be derived from existing one.
>>>>
>>> The ABI was added in the previous patch so that we can present the
>>> sensor orientation information to userspace even in the case of device
>>> tree.
>>
>> If the main reason for implementing a new ABI is to support DT platforms,
>> Why not implement a version of _PLD for device tree ? Don't you think it
>> would be much better than adding a new ABI to export redundant information ?
>>
>
> IMO the mounting matrix is more consistent with the IIO ABIs. Although
> I have no issue with repicating _PLD for device tree if people agree
> that it is better.
>
>> Also the location information of the device is not just specific to iio
>> drivers. You should consider that we would have similar requirements for
>> devices implemented as input or platform drivers.
>
> The upstream standard for those sensors where the orientation matters
> (accelerometer, gyro, compass) is IIO.
It's probably worth pull Dmitry in on this conversation as well (and maybe
the input list). cc'd.
For reference of those who haven't seen this before. The question here is about
exposing the sensor mounting matrix ( orientation of the sensor frame of reference
relative to some 'magic' orientation - probably the PCB )
Note I'd also throw in here that I'd argue for a combined R T matrix
e.g. [R T] so as to cover cases where we care about the position as well as the
orientation relative to some reference point. A class case in point would
be rotation measurement from offset accelerometers.
Could go with a full projective geometry matrix, but probably better to keep it
in some base scale e.g.
[R T]
[000 1]
but not bother exporting the 000 1 as it'll always be the same.
Note I've written this email whilst without net access (free wifi at
Stansted airport is rubbish!) so haven't checked out the existing ACPI ABI.
Will try and remember to do this when I get a moment with working internet.
Jonathan
>
> Granted, there are other device types for which the orientation
> information may be useful (e.g. camera). However the actual
> interpretation and action to be taken is different for each subsystem
> (e.g. in the camera case do the correction via V4L2_CID_HFLIP /
> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem
> level in a way consistent with the subsystem's ABIs.
>
^ permalink raw reply
* Re: [PATCH v7 05/15] dt-bindings: Document the STM32 reset bindings
From: Maxime Coquelin @ 2015-05-04 11:11 UTC (permalink / raw)
To: Philipp Zabel
Cc: Daniel Thompson, Uwe Kleine-König, Andreas Färber,
Geert Uytterhoeven, Rob Herring, Linus Walleij, Arnd Bergmann,
Stefan Agner, Peter Meerwald, Paul Bolle, Peter Hurley,
Andy Shevchenko, Chanwoo Choi, Russell King, Daniel Lezcano,
Joe Perches, Vladimir Zapolskiy, Jonathan Corbet, Pawel Moll,
Mark Rutland, Ian Campbell
In-Reply-To: <1430735320.3722.34.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2015-05-04 12:28 GMT+02:00 Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>:
> Am Samstag, den 02.05.2015, 11:01 +0100 schrieb Daniel Thompson:
>> On 02/05/15 08:55, Maxime Coquelin wrote:
>> > 2015-05-01 10:08 GMT+02:00 Daniel Thompson <daniel.thompson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> [...]
>> >> Do you intend the clock driver to use the same compatible string (given it
>> >> is the same bit of hardware).
>> >>
>> >> If so, is it better to use st,stm32f4-rcc here? It seems unlikey to me that
>> >> the register layout of the PLLs and dividers can be the same on the f7 parts
>> >> (and later).
>> >
>> > I agree we need a compatible dedicate to f4 series for clocks, and
>> > maybe even one for f429 (to be checked).
>> > For the reset part, we don't have this need.
>> >
>> > So either we use only "st,stm32f4" as you suggest, or we can have both
>> > in device tree:
>> >
>> > rcc: reset@40023800 {
>> > #reset-cells = <1>;
>> > compatible = "st,stm32f4-rcc", "st,stm32-rcc";
>> > reg = <0x40023800 0x400>;
>> > };
>> >
>> > What do you think?
>>
>> Having both makes sense. The reset driver probably doesn't care about
>> differences between F4 and F7 (I know very little about F7 but I can't
>> think of any obvious h/ware evolution that would confuse the current
>> reset driver).
>
> Seconded, this is exactly the way compatible string lists are supposed
> to be used.
Ok good, so we all agree.
I propose I keep "st,stm32-rcc" only for this series, as it does not
contain the clock driver.
The series adding the clock driver will add "st,stm32f4-rcc", or
"st,stm32f429-rcc" depending on clock driver needs.
Thanks,
Maxime
>
> [...]
>
> regards
> Philipp
>
^ permalink raw reply
* Re: [PATCH v3 3/3] proc: add kpageidle file
From: Minchan Kim @ 2015-05-04 10:54 UTC (permalink / raw)
To: Vladimir Davydov
Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Greg Thelen,
Michel Lespinasse, David Rientjes, Pavel Emelyanov,
Cyrill Gorcunov, Jonathan Corbet, linux-api, linux-doc, linux-mm,
cgroups, linux-kernel, Rik van Riel, Hugh Dickins,
Christoph Lameter, Paul E. McKenney, Peter Zijlstra
In-Reply-To: <20150504094938.GB4197@esperanza>
On Mon, May 04, 2015 at 12:49:39PM +0300, Vladimir Davydov wrote:
> On Mon, May 04, 2015 at 12:17:22PM +0900, Minchan Kim wrote:
> > On Thu, Apr 30, 2015 at 05:50:55PM +0300, Vladimir Davydov wrote:
> > > On Thu, Apr 30, 2015 at 05:25:31PM +0900, Minchan Kim wrote:
> > > > On Wed, Apr 29, 2015 at 12:12:48PM +0300, Vladimir Davydov wrote:
> > > > > On Wed, Apr 29, 2015 at 01:35:36PM +0900, Minchan Kim wrote:
> > > > > > On Tue, Apr 28, 2015 at 03:24:42PM +0300, Vladimir Davydov wrote:
> > > > > > > +#ifdef CONFIG_IDLE_PAGE_TRACKING
> > > > > > > +static struct page *kpageidle_get_page(unsigned long pfn)
> > > > > > > +{
> > > > > > > + struct page *page;
> > > > > > > +
> > > > > > > + if (!pfn_valid(pfn))
> > > > > > > + return NULL;
> > > > > > > + page = pfn_to_page(pfn);
> > > > > > > + /*
> > > > > > > + * We are only interested in user memory pages, i.e. pages that are
> > > > > > > + * allocated and on an LRU list.
> > > > > > > + */
> > > > > > > + if (!page || page_count(page) == 0 || !PageLRU(page))
> > > > > > > + return NULL;
> > > > > > > + if (!get_page_unless_zero(page))
> > > > > > > + return NULL;
> > > > > > > + if (unlikely(!PageLRU(page))) {
> > > > > >
> > > > > > What lock protect the check PageLRU?
> > > > > > If it is racing ClearPageLRU, what happens?
> > > > >
> > > > > If we hold a reference to a page and see that it's on an LRU list, it
> > > > > will surely remain a user memory page at least until we release the
> > > > > reference to it, so it must be safe to play with idle/young flags. If we
> > > >
> > > > The problem is that you pass the page in rmap reverse logic(ie, page_referenced)
> > > > once you judge it's LRU page so if it is false-positive, what happens?
> > > > A question is SetPageLRU, PageLRU, ClearPageLRU keeps memory ordering?
> > > > IOW, all of fields from struct page rmap can acccess should be set up completely
> > > > before LRU checking. Otherwise, something will be broken.
> > >
> > > So, basically you are concerned about the case when we encounter a
> > > freshly allocated page, which has PG_lru bit set and it's going to
> > > become anonymous, but it is still in the process of rmap initialization,
> > > i.e. its ->mapping or ->mapcount may still be uninitialized, right?
> > >
> > > AFAICS, page_referenced should handle such pages fine. Look, it only
> > > needs ->index, ->mapping, and ->mapcount.
> > >
> > > If ->mapping is unset, than it is NULL and rmap_walk_anon_lock ->
> > > page_lock_anon_vma_read will return NULL so that rmap_walk will be a
> > > no-op.
> > >
> > > If ->index is not initialized, than at worst we will go to
> > > anon_vma_interval_tree_foreach over a wrong interval, in which case we
> > > will see that the page is actually not mapped in page_referenced_one ->
> > > page_check_address and again do nothing.
> > >
> > > If ->mapcount is not initialized it is -1, and page_lock_anon_vma_read
> > > will return NULL, just as it does in case ->mapping = NULL.
> > >
> > > For file pages, we always take PG_locked before checking ->mapping, so
> > > it must be valid.
> > >
> > > Thanks,
> > > Vladimir
> >
> >
> > do_anonymous_page
> > page_add_new_anon_rmap
> > atomic_set(&page->_mapcount, 0);
> > __page_set_anon_rmap
> > anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
> > page->mapping = (struct address_space *) anon_vma;
> > page->index = linear_page_index(vma, address);
> > lru_cache_add
> > __pagevec_lru_add_fn
> > SetPageLRU(page);
> >
> > During the procedure, there is no lock to prevent race. Then, at least,
> > we need a write memory barrier to guarantee other fields set up before
> > SetPageLRU. (Of course, PageLRU should have read-memory barrier to work
> > well) But I can't find any barrier, either.
> >
> > IOW, any fields you said could be out of order store without any lock or
> > memory barrier. You might argue atomic op is a barrier on x86 but it
> > doesn't guarantee other arches work like that so we need explict momory
> > barrier or lock.
> >
> > Let's have a theoretical example.
> >
> > CPU 0 CPU 1
> >
> > do_anonymous_page
> > __page_set_anon_rmap
> > /* out of order happened so SetPageLRU is done ahead */
> > SetPageLRU(page)
> > /* Compilr changed store operation like below */
>
> But it couldn't. Quoting Documentation/atomic_ops.txt:
>
> Properly aligned pointers, longs, ints, and chars (and unsigned
> equivalents) may be atomically loaded from and stored to in the same
> sense as described for atomic_read() and atomic_set().
>
> __page_set_anon_rmap sets page->mapping using the following expression:
>
> anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
> page->mapping = (struct address_space *) anon_vma;
>
> and it can't be split, i.e. if one concurrently reads page->mapping
> he/she will see either NULL or (anon_vma+PAGE_MAPPING_ANON), and there
> can't be any intermediate result in page->mapping, such as anon_vma or
> PAGE_MAPPING_ANON, because one doesn't expect
>
> atomic_set(&p, a + b);
>
> to behave like
>
> atomic_set(&p, a);
> atomic_set(&p, atomic_read(&p) + b);
When I parsed the documentation, I understand it that each of words
store/load operation is atomically loaded or stored, not forcing to
preventing split.
Hmm, but it's really important part in this patchset's implementation
so I want to confirm during review process.
I don't have a worry even if I am not a expert about that part because
I know other experts. ;-) Ccing them.
What I want to know is as follows,
In do_anonymous_page, there is following peice of code.
page_add_new_anon_rmap
__page_set_anon_rmap
anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
page->mapping = (struct address_space *) anon_vma;
lru_cache_add_active_or_unevictable
__lru_cache_add
__pagevec_lru_add_fn
SetPageLRU(page);
>From page->mapping to SetPageLRU, there is no explict lock and memory
barrier.
As counterpart, kpageidle can pass page in page_referenced once it judges
out the page has PG_lru with PageLRU check but my concern is that it
judges without any lock or barrier like below.
static struct page *kpageidle_get_page(unsigned long pfn)
{
struct page *page;
if (!pfn_valid(pfn))
return NULL;
page = pfn_to_page(pfn);
/*
* We are only interested in user memory pages, i.e. pages that are
* allocated and on an LRU list.
*/
if (!page || page_count(page) == 0 || !PageLRU(page))
return NULL;
if (!get_page_unless_zero(page))
return NULL;
if (unlikely(!PageLRU(page))) {
put_page(page);
return NULL;
}
return page;
}
So, I guess once below compiler optimization happens in __page_set_anon_rmap,
it could be corrupt in page_refernced.
__page_set_anon_rmap:
page->mapping = (struct address_space *) anon_vma;
page->mapping = (struct address_space *)((void *)page_mapping + PAGE_MAPPING_ANON);
Because page_referenced checks it with PageAnon which has no memory barrier.
So if above compiler optimization happens, page_referenced can pass the anon
page in rmap_walk_file, not ramp_walk_anon. It's my theory. :)
But Vladimir said above above compiler optimization cannot happen because
it's aligned pointer and Documentation/atomic_ops.txt said it couldn't happen.
Thanks for looking this.
> > page->mapping = (struct address_space *) anon_vma;
> > /* Big stall happens */
> > /* idletacking judged it as LRU page so pass the page
> > in page_reference */
> > page_refernced
> > page_rmapping return true because
> > page->mapping has some vaule but not complete
> > so it calls rmap_walk_file.
> > it's okay to pass non-completed anon page in rmap_walk_file?
> >
> > page->mapping = (struct address_space *)
> > ((void *)page_mapping + PAGE_MAPPING_ANON);
>
>
> Thanks,
> Vladimir
>
> > page->mapping = (struct address_space *) anon_vma;
> > /* Big stall happens */
> > /* idletacking judged it as LRU page so pass the page
> > in page_reference */
> > page_refernced
> > page_rmapping return true because
> > page->mapping has some vaule but not complete
> > so it calls rmap_walk_file.
> > it's okay to pass non-completed anon page in rmap_walk_file?
> >
> > page->mapping = (struct address_space *)
> > ((void *)page_mapping + PAGE_MAPPING_ANON);
> >
> > It's too theoretical so it might be hard to happen in real practice.
> > My point is there is nothing to prevent explict race.
> > Even if there is no problem with other lock, it's fragile.
> > Do I miss something?
> >
> > I think general way to handle PageLRU are ahead isolation or zone->lru_lock.
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v7 05/15] dt-bindings: Document the STM32 reset bindings
From: Philipp Zabel @ 2015-05-04 10:28 UTC (permalink / raw)
To: Daniel Thompson
Cc: Maxime Coquelin, Uwe Kleine-König, Andreas Färber,
Geert Uytterhoeven, Rob Herring, Linus Walleij, Arnd Bergmann,
Stefan Agner, Peter Meerwald, Paul Bolle, Peter Hurley,
Andy Shevchenko, Chanwoo Choi, Russell King, Daniel Lezcano,
Joe Perches, Vladimir Zapolskiy, Jonathan Corbet, Pawel Moll,
Mark Rutland, Ian Campbell
In-Reply-To: <5544A069.5000808@linaro.org>
Am Samstag, den 02.05.2015, 11:01 +0100 schrieb Daniel Thompson:
> On 02/05/15 08:55, Maxime Coquelin wrote:
> > 2015-05-01 10:08 GMT+02:00 Daniel Thompson <daniel.thompson@linaro.org>:
[...]
> >> Do you intend the clock driver to use the same compatible string (given it
> >> is the same bit of hardware).
> >>
> >> If so, is it better to use st,stm32f4-rcc here? It seems unlikey to me that
> >> the register layout of the PLLs and dividers can be the same on the f7 parts
> >> (and later).
> >
> > I agree we need a compatible dedicate to f4 series for clocks, and
> > maybe even one for f429 (to be checked).
> > For the reset part, we don't have this need.
> >
> > So either we use only "st,stm32f4" as you suggest, or we can have both
> > in device tree:
> >
> > rcc: reset@40023800 {
> > #reset-cells = <1>;
> > compatible = "st,stm32f4-rcc", "st,stm32-rcc";
> > reg = <0x40023800 0x400>;
> > };
> >
> > What do you think?
>
> Having both makes sense. The reset driver probably doesn't care about
> differences between F4 and F7 (I know very little about F7 but I can't
> think of any obvious h/ware evolution that would confuse the current
> reset driver).
Seconded, this is exactly the way compatible string lists are supposed
to be used.
[...]
regards
Philipp
^ permalink raw reply
* Re: [PATCH v3 3/3] proc: add kpageidle file
From: Vladimir Davydov @ 2015-05-04 9:49 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Greg Thelen,
Michel Lespinasse, David Rientjes, Pavel Emelyanov,
Cyrill Gorcunov, Jonathan Corbet,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
cgroups-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150504031722.GA2768@blaptop>
On Mon, May 04, 2015 at 12:17:22PM +0900, Minchan Kim wrote:
> On Thu, Apr 30, 2015 at 05:50:55PM +0300, Vladimir Davydov wrote:
> > On Thu, Apr 30, 2015 at 05:25:31PM +0900, Minchan Kim wrote:
> > > On Wed, Apr 29, 2015 at 12:12:48PM +0300, Vladimir Davydov wrote:
> > > > On Wed, Apr 29, 2015 at 01:35:36PM +0900, Minchan Kim wrote:
> > > > > On Tue, Apr 28, 2015 at 03:24:42PM +0300, Vladimir Davydov wrote:
> > > > > > +#ifdef CONFIG_IDLE_PAGE_TRACKING
> > > > > > +static struct page *kpageidle_get_page(unsigned long pfn)
> > > > > > +{
> > > > > > + struct page *page;
> > > > > > +
> > > > > > + if (!pfn_valid(pfn))
> > > > > > + return NULL;
> > > > > > + page = pfn_to_page(pfn);
> > > > > > + /*
> > > > > > + * We are only interested in user memory pages, i.e. pages that are
> > > > > > + * allocated and on an LRU list.
> > > > > > + */
> > > > > > + if (!page || page_count(page) == 0 || !PageLRU(page))
> > > > > > + return NULL;
> > > > > > + if (!get_page_unless_zero(page))
> > > > > > + return NULL;
> > > > > > + if (unlikely(!PageLRU(page))) {
> > > > >
> > > > > What lock protect the check PageLRU?
> > > > > If it is racing ClearPageLRU, what happens?
> > > >
> > > > If we hold a reference to a page and see that it's on an LRU list, it
> > > > will surely remain a user memory page at least until we release the
> > > > reference to it, so it must be safe to play with idle/young flags. If we
> > >
> > > The problem is that you pass the page in rmap reverse logic(ie, page_referenced)
> > > once you judge it's LRU page so if it is false-positive, what happens?
> > > A question is SetPageLRU, PageLRU, ClearPageLRU keeps memory ordering?
> > > IOW, all of fields from struct page rmap can acccess should be set up completely
> > > before LRU checking. Otherwise, something will be broken.
> >
> > So, basically you are concerned about the case when we encounter a
> > freshly allocated page, which has PG_lru bit set and it's going to
> > become anonymous, but it is still in the process of rmap initialization,
> > i.e. its ->mapping or ->mapcount may still be uninitialized, right?
> >
> > AFAICS, page_referenced should handle such pages fine. Look, it only
> > needs ->index, ->mapping, and ->mapcount.
> >
> > If ->mapping is unset, than it is NULL and rmap_walk_anon_lock ->
> > page_lock_anon_vma_read will return NULL so that rmap_walk will be a
> > no-op.
> >
> > If ->index is not initialized, than at worst we will go to
> > anon_vma_interval_tree_foreach over a wrong interval, in which case we
> > will see that the page is actually not mapped in page_referenced_one ->
> > page_check_address and again do nothing.
> >
> > If ->mapcount is not initialized it is -1, and page_lock_anon_vma_read
> > will return NULL, just as it does in case ->mapping = NULL.
> >
> > For file pages, we always take PG_locked before checking ->mapping, so
> > it must be valid.
> >
> > Thanks,
> > Vladimir
>
>
> do_anonymous_page
> page_add_new_anon_rmap
> atomic_set(&page->_mapcount, 0);
> __page_set_anon_rmap
> anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
> page->mapping = (struct address_space *) anon_vma;
> page->index = linear_page_index(vma, address);
> lru_cache_add
> __pagevec_lru_add_fn
> SetPageLRU(page);
>
> During the procedure, there is no lock to prevent race. Then, at least,
> we need a write memory barrier to guarantee other fields set up before
> SetPageLRU. (Of course, PageLRU should have read-memory barrier to work
> well) But I can't find any barrier, either.
>
> IOW, any fields you said could be out of order store without any lock or
> memory barrier. You might argue atomic op is a barrier on x86 but it
> doesn't guarantee other arches work like that so we need explict momory
> barrier or lock.
>
> Let's have a theoretical example.
>
> CPU 0 CPU 1
>
> do_anonymous_page
> __page_set_anon_rmap
> /* out of order happened so SetPageLRU is done ahead */
> SetPageLRU(page)
> /* Compilr changed store operation like below */
But it couldn't. Quoting Documentation/atomic_ops.txt:
Properly aligned pointers, longs, ints, and chars (and unsigned
equivalents) may be atomically loaded from and stored to in the same
sense as described for atomic_read() and atomic_set().
__page_set_anon_rmap sets page->mapping using the following expression:
anon_vma = (void *) anon_vma + PAGE_MAPPING_ANON;
page->mapping = (struct address_space *) anon_vma;
and it can't be split, i.e. if one concurrently reads page->mapping
he/she will see either NULL or (anon_vma+PAGE_MAPPING_ANON), and there
can't be any intermediate result in page->mapping, such as anon_vma or
PAGE_MAPPING_ANON, because one doesn't expect
atomic_set(&p, a + b);
to behave like
atomic_set(&p, a);
atomic_set(&p, atomic_read(&p) + b);
Thanks,
Vladimir
> page->mapping = (struct address_space *) anon_vma;
> /* Big stall happens */
> /* idletacking judged it as LRU page so pass the page
> in page_reference */
> page_refernced
> page_rmapping return true because
> page->mapping has some vaule but not complete
> so it calls rmap_walk_file.
> it's okay to pass non-completed anon page in rmap_walk_file?
>
> page->mapping = (struct address_space *)
> ((void *)page_mapping + PAGE_MAPPING_ANON);
>
> It's too theoretical so it might be hard to happen in real practice.
> My point is there is nothing to prevent explict race.
> Even if there is no problem with other lock, it's fragile.
> Do I miss something?
>
> I think general way to handle PageLRU are ahead isolation or zone->lru_lock.
^ permalink raw reply
* Re: [RFC PATCH 3/3] iio: derive the mounting matrix from ACPI _PLD objects
From: Octavian Purdila @ 2015-05-04 8:21 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Robert Moore, Rafael J Wysocki, Len Brown,
linux-api-u79uwXL29TY76Z2rM5mHXA, lkml,
linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Srinivas Pandruvada
In-Reply-To: <5546C72E.2040101-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On Mon, May 4, 2015 at 4:11 AM, Sathyanarayanan Kuppuswamy
<sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> Hi Octavian,
>
> On 04/27/2015 07:23 PM, Octavian Purdila wrote:
>>
>> On Tue, Apr 28, 2015 at 12:57 AM, sathyanarayanan kuppuswamy
>> <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>
>>> Hi
>>>
>>> On 04/27/2015 08:54 AM, Octavian Purdila wrote:
>>>>
>>>> On Mon, Apr 27, 2015 at 6:42 PM, Kuppuswamy Sathyanarayanan
>>>> <sathyanarayanan.kuppuswamy-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>>>>
>>>>> Since Acpi framework already exports this info to user space, Why not
>>>>> do
>>>>> this derivation in user space code ? Why do we need new ABI, if the
>>>>> same
>>>>> can be derived from existing one.
>>>>>
>>>> The ABI was added in the previous patch so that we can present the
>>>> sensor orientation information to userspace even in the case of device
>>>> tree.
>>>
>>> If the main reason for implementing a new ABI is to support DT platforms,
>>> Why not implement a version of _PLD for device tree ? Don't you think it
>>> would be much better than adding a new ABI to export redundant
>>> information ?
>>>
>> IMO the mounting matrix is more consistent with the IIO ABIs. Although
>> I have no issue with repicating _PLD for device tree if people agree
>> that it is better.
>
> Since your main issue is, device tree lacking ABI to specify location
> information, you should consider fixing it there. Let's wait for others
> comment on this.
>
> If you think mounting matrix provides more information than what is
> supported
> by _PLD, then we should consider implementing another ABI. AFAIK, that is
> not
> the case here.
>
> Adding adding a new ABI to represent the information that can be derived
> from existing ABI does not seem to be useful.
AFAICS the ACPI _PLD information is not (yet) exported to userspace. This patch:
http://marc.info/?t=140795040700003&r=1&w=2
does not seem to be merged upstream. So there is no existing ABI to
derive from :)
>>
>>
>>> Also the location information of the device is not just specific to iio
>>> drivers. You should consider that we would have similar requirements for
>>> devices implemented as input or platform drivers.
>>
>> The upstream standard for those sensors where the orientation matters
>> (accelerometer, gyro, compass) is IIO.
>>
>> Granted, there are other device types for which the orientation
>> information may be useful (e.g. camera). However the actual
>> interpretation and action to be taken is different for each subsystem
>> (e.g. in the camera case do the correction via V4L2_CID_HFLIP /
>> V4L2_CID_VFLIP) so I think it is better to expose it at the subsystem
>> level in a way consistent with the subsystem's ABIs.
>
> I agree that location information is used differently at different
> sub systems. But my question is why we need a new ABI ?
>
> Why not handle it in user space ?
>
> -
^ permalink raw reply
* [PATCH 4/4] media/vivid: Code cleanout
From: Ricardo Ribalda Delgado @ 2015-05-04 8:07 UTC (permalink / raw)
To: Hans Verkuil, Mauro Carvalho Chehab, Laurent Pinchart,
Sakari Ailus, Ramakrishnan Muthukrishnan,
linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Ricardo Ribalda Delgado
In-Reply-To: <1430726852-11715-1-git-send-email-ricardo.ribalda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Remove code duplication by merging two cases in a switch.
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/media/platform/vivid/vivid-tpg.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/media/platform/vivid/vivid-tpg.c b/drivers/media/platform/vivid/vivid-tpg.c
index 2e5129a6bc2f..8cac0bdefd9a 100644
--- a/drivers/media/platform/vivid/vivid-tpg.c
+++ b/drivers/media/platform/vivid/vivid-tpg.c
@@ -294,6 +294,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
}
switch (fourcc) {
+ case V4L2_PIX_FMT_GREY:
case V4L2_PIX_FMT_RGB332:
tpg->twopixelsize[0] = 2;
break;
@@ -333,9 +334,6 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
case V4L2_PIX_FMT_YUV32:
tpg->twopixelsize[0] = 2 * 4;
break;
- case V4L2_PIX_FMT_GREY:
- tpg->twopixelsize[0] = 2;
- break;
case V4L2_PIX_FMT_NV12:
case V4L2_PIX_FMT_NV21:
case V4L2_PIX_FMT_NV12M:
--
2.1.4
^ permalink raw reply related
* [PATCH 3/4] media/vivid: Add support for Y16_BE format
From: Ricardo Ribalda Delgado @ 2015-05-04 8:07 UTC (permalink / raw)
To: Hans Verkuil, Mauro Carvalho Chehab, Laurent Pinchart,
Sakari Ailus, Ramakrishnan Muthukrishnan, linux-media,
linux-kernel, linux-api
Cc: Ricardo Ribalda Delgado
In-Reply-To: <1430726852-11715-1-git-send-email-ricardo.ribalda@gmail.com>
Support for V4L2_PIX_FMT_Y16_BE, a 16 bit big endian greyscale format.
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
drivers/media/platform/vivid/vivid-tpg.c | 9 ++++++++-
drivers/media/platform/vivid/vivid-vid-common.c | 8 ++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/vivid/vivid-tpg.c b/drivers/media/platform/vivid/vivid-tpg.c
index 9e50303a19c5..2e5129a6bc2f 100644
--- a/drivers/media/platform/vivid/vivid-tpg.c
+++ b/drivers/media/platform/vivid/vivid-tpg.c
@@ -221,6 +221,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
case V4L2_PIX_FMT_ABGR32:
case V4L2_PIX_FMT_GREY:
case V4L2_PIX_FMT_Y16:
+ case V4L2_PIX_FMT_Y16_BE:
tpg->is_yuv = false;
break;
case V4L2_PIX_FMT_YUV444:
@@ -315,6 +316,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
case V4L2_PIX_FMT_YUV555:
case V4L2_PIX_FMT_YUV565:
case V4L2_PIX_FMT_Y16:
+ case V4L2_PIX_FMT_Y16_BE:
tpg->twopixelsize[0] = 2 * 2;
break;
case V4L2_PIX_FMT_RGB24:
@@ -715,7 +717,8 @@ static void precalculate_color(struct tpg_data *tpg, int k)
b <<= 4;
}
if (tpg->qual == TPG_QUAL_GRAY || tpg->fourcc == V4L2_PIX_FMT_GREY ||
- tpg->fourcc == V4L2_PIX_FMT_Y16) {
+ tpg->fourcc == V4L2_PIX_FMT_Y16 ||
+ tpg->fourcc == V4L2_PIX_FMT_Y16_BE) {
/* Rec. 709 Luma function */
/* (0.2126, 0.7152, 0.0722) * (255 * 256) */
r = g = b = (13879 * r + 46688 * g + 4713 * b) >> 16;
@@ -902,6 +905,10 @@ static void gen_twopix(struct tpg_data *tpg,
buf[0][offset] = 0;
buf[0][offset+1] = r_y;
break;
+ case V4L2_PIX_FMT_Y16_BE:
+ buf[0][offset] = r_y;
+ buf[0][offset+1] = 0;
+ break;
case V4L2_PIX_FMT_YUV422P:
case V4L2_PIX_FMT_YUV420:
case V4L2_PIX_FMT_YUV420M:
diff --git a/drivers/media/platform/vivid/vivid-vid-common.c b/drivers/media/platform/vivid/vivid-vid-common.c
index 96ccd3c38dd2..45f10a7f9b46 100644
--- a/drivers/media/platform/vivid/vivid-vid-common.c
+++ b/drivers/media/platform/vivid/vivid-vid-common.c
@@ -197,6 +197,14 @@ struct vivid_fmt vivid_formats[] = {
.buffers = 1,
},
{
+ .fourcc = V4L2_PIX_FMT_Y16_BE,
+ .vdownsampling = { 1 },
+ .bit_depth = { 16 },
+ .is_yuv = true,
+ .planes = 1,
+ .buffers = 1,
+ },
+ {
.fourcc = V4L2_PIX_FMT_RGB332, /* rrrgggbb */
.vdownsampling = { 1 },
.bit_depth = { 8 },
--
2.1.4
^ permalink raw reply related
* [PATCH 2/4] media/v4l2-core: Add support for V4L2_PIX_FMT_Y16_BE
From: Ricardo Ribalda Delgado @ 2015-05-04 8:07 UTC (permalink / raw)
To: Hans Verkuil, Mauro Carvalho Chehab, Laurent Pinchart,
Sakari Ailus, Ramakrishnan Muthukrishnan, linux-media,
linux-kernel, linux-api
Cc: Ricardo Ribalda Delgado
In-Reply-To: <1430726852-11715-1-git-send-email-ricardo.ribalda@gmail.com>
16 bit greyscale format, structured in Big Endian. Such a format can be
converted into a PMN image just by adding a header.
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
include/uapi/linux/videodev2.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 14766029bf49..136df7ccfedc 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1146,6 +1146,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
case V4L2_PIX_FMT_Y10: descr = "10-bit Greyscale"; break;
case V4L2_PIX_FMT_Y12: descr = "12-bit Greyscale"; break;
case V4L2_PIX_FMT_Y16: descr = "16-bit Greyscale"; break;
+ case V4L2_PIX_FMT_Y16_BE: descr = "16-bit Greyscale BE"; break;
case V4L2_PIX_FMT_Y10BPACK: descr = "10-bit Greyscale (Packed)"; break;
case V4L2_PIX_FMT_PAL8: descr = "8-bit Palette"; break;
case V4L2_PIX_FMT_UV8: descr = "8-bit Chrominance UV 4-4"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index fa376f7666ba..3f01d7bbbc77 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -404,6 +404,7 @@ struct v4l2_pix_format {
#define V4L2_PIX_FMT_Y10 v4l2_fourcc('Y', '1', '0', ' ') /* 10 Greyscale */
#define V4L2_PIX_FMT_Y12 v4l2_fourcc('Y', '1', '2', ' ') /* 12 Greyscale */
#define V4L2_PIX_FMT_Y16 v4l2_fourcc('Y', '1', '6', ' ') /* 16 Greyscale */
+#define V4L2_PIX_FMT_Y16_BE v4l2_fourcc_be('Y', '1', '6', ' ') /* 16 Greyscale BE */
/* Grey bit-packed formats */
#define V4L2_PIX_FMT_Y10BPACK v4l2_fourcc('Y', '1', '0', 'B') /* 10 Greyscale bit-packed */
--
2.1.4
^ permalink raw reply related
* [PATCH 1/4] media/vivid: Add support for Y16 format
From: Ricardo Ribalda Delgado @ 2015-05-04 8:07 UTC (permalink / raw)
To: Hans Verkuil, Mauro Carvalho Chehab, Laurent Pinchart,
Sakari Ailus, Ramakrishnan Muthukrishnan,
linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Ricardo Ribalda Delgado
In-Reply-To: <1430726852-11715-1-git-send-email-ricardo.ribalda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Support for V4L2_PIX_FMT_Y16, a 16 bit greyscale format.
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/media/platform/vivid/vivid-tpg.c | 9 ++++++++-
drivers/media/platform/vivid/vivid-vid-common.c | 8 ++++++++
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/vivid/vivid-tpg.c b/drivers/media/platform/vivid/vivid-tpg.c
index 4df755aa7e48..9e50303a19c5 100644
--- a/drivers/media/platform/vivid/vivid-tpg.c
+++ b/drivers/media/platform/vivid/vivid-tpg.c
@@ -220,6 +220,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
case V4L2_PIX_FMT_ARGB32:
case V4L2_PIX_FMT_ABGR32:
case V4L2_PIX_FMT_GREY:
+ case V4L2_PIX_FMT_Y16:
tpg->is_yuv = false;
break;
case V4L2_PIX_FMT_YUV444:
@@ -313,6 +314,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
case V4L2_PIX_FMT_YUV444:
case V4L2_PIX_FMT_YUV555:
case V4L2_PIX_FMT_YUV565:
+ case V4L2_PIX_FMT_Y16:
tpg->twopixelsize[0] = 2 * 2;
break;
case V4L2_PIX_FMT_RGB24:
@@ -712,7 +714,8 @@ static void precalculate_color(struct tpg_data *tpg, int k)
g <<= 4;
b <<= 4;
}
- if (tpg->qual == TPG_QUAL_GRAY || tpg->fourcc == V4L2_PIX_FMT_GREY) {
+ if (tpg->qual == TPG_QUAL_GRAY || tpg->fourcc == V4L2_PIX_FMT_GREY ||
+ tpg->fourcc == V4L2_PIX_FMT_Y16) {
/* Rec. 709 Luma function */
/* (0.2126, 0.7152, 0.0722) * (255 * 256) */
r = g = b = (13879 * r + 46688 * g + 4713 * b) >> 16;
@@ -895,6 +898,10 @@ static void gen_twopix(struct tpg_data *tpg,
case V4L2_PIX_FMT_GREY:
buf[0][offset] = r_y;
break;
+ case V4L2_PIX_FMT_Y16:
+ buf[0][offset] = 0;
+ buf[0][offset+1] = r_y;
+ break;
case V4L2_PIX_FMT_YUV422P:
case V4L2_PIX_FMT_YUV420:
case V4L2_PIX_FMT_YUV420M:
diff --git a/drivers/media/platform/vivid/vivid-vid-common.c b/drivers/media/platform/vivid/vivid-vid-common.c
index 6ba874420485..96ccd3c38dd2 100644
--- a/drivers/media/platform/vivid/vivid-vid-common.c
+++ b/drivers/media/platform/vivid/vivid-vid-common.c
@@ -189,6 +189,14 @@ struct vivid_fmt vivid_formats[] = {
.buffers = 1,
},
{
+ .fourcc = V4L2_PIX_FMT_Y16,
+ .vdownsampling = { 1 },
+ .bit_depth = { 16 },
+ .is_yuv = true,
+ .planes = 1,
+ .buffers = 1,
+ },
+ {
.fourcc = V4L2_PIX_FMT_RGB332, /* rrrgggbb */
.vdownsampling = { 1 },
.bit_depth = { 8 },
--
2.1.4
^ permalink raw reply related
* [PATCH 0/4] Add support for V4L2_PIX_FMT_Y16_BE
From: Ricardo Ribalda Delgado @ 2015-05-04 8:07 UTC (permalink / raw)
To: Hans Verkuil, Mauro Carvalho Chehab, Laurent Pinchart,
Sakari Ailus, Ramakrishnan Muthukrishnan,
linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
Cc: Ricardo Ribalda Delgado
New pixel format type Y16_BE (16 bits greyscale big-endian).
Once I get the fist feedback on this patch I will send the patches for
v4lconvert and qv4l2.
Thanks
Ricardo Ribalda Delgado (4):
media/vivid: Add support for Y16 format
media/v4l2-core: Add support for V4L2_PIX_FMT_Y16_BE
media/vivid: Add support for Y16_BE format
media/vivid: Code cleanout
drivers/media/platform/vivid/vivid-tpg.c | 20 ++++++++++++++++----
drivers/media/platform/vivid/vivid-vid-common.c | 16 ++++++++++++++++
drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
include/uapi/linux/videodev2.h | 1 +
4 files changed, 34 insertions(+), 4 deletions(-)
--
2.1.4
^ permalink raw reply
* Re: [PATCH net-next] tcp: provide SYN headers for passive connections
From: Michael Kerrisk (man-pages) @ 2015-05-04 6:47 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric B Munson, Tom Herbert, David S. Miller, Linux API, netdev
In-Reply-To: <1430714086.3711.165.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
Eric,
On 4 May 2015 at 06:34, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> This patch allows a server application to get the TCP SYN headers for
> its passive connections. This is useful if the server is doing
> fingerprinting of clients based on SYN packet contents.
>
> Two socket options are added: TCP_SAVE_SYN and TCP_SAVED_SYN.
>
> The first is used on a socket to enable saving the SYN headers
> for child connections. This can be set before or after the listen()
> call.
>
> The latter is used to retrieve the SYN headers for passive connections,
> if the parent listener has enabled TCP_SAVE_SYN.
>
> TCP_SAVED_SYN is read once, it frees the saved SYN headers.
>
> The data returned in TCP_SAVED_SYN are network (IPv4/IPv6) and TCP
> headers.
This description is a little thin, so I'm unclear on one or two
points. TCP_SAVE_SYN is clearly applied to the listening socket. But
what about TCP_SAVED_SYN? Is that applied to the connected socket
returned by accept()?
The highly similar naming of these two seems unfortunate. At the very
least, it makes for easy confusion in conversations about the two
options. It would be better to have names that were more distinct.
Perhaps the latter could be TCP_CONN_SYN or TCP_CONN_SAVED_SYN, for
example?
Thanks,
Michael
> Original patch was written by Tom Herbert, I changed it to not hold
> a full skb (and associated dst and conntracking reference).
>
> We have used such patch for about 3 years at Google.
>
> Signed-off-by: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> include/linux/tcp.h | 8 ++++++++
> include/net/request_sock.h | 4 +++-
> include/uapi/linux/tcp.h | 2 ++
> net/ipv4/tcp.c | 35 +++++++++++++++++++++++++++++++++++
> net/ipv4/tcp_input.c | 18 ++++++++++++++++++
> net/ipv4/tcp_ipv4.c | 1 +
> net/ipv4/tcp_minisocks.c | 3 +++
> 7 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 3b2911502a8c..e6fb5df22db1 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -199,6 +199,7 @@ struct tcp_sock {
> syn_fastopen:1, /* SYN includes Fast Open option */
> syn_fastopen_exp:1,/* SYN includes Fast Open exp. option */
> syn_data_acked:1,/* data in SYN is acked by SYN-ACK */
> + save_syn:1, /* Save headers of SYN packet */
> is_cwnd_limited:1;/* forward progress limited by snd_cwnd? */
> u32 tlp_high_seq; /* snd_nxt at the time of TLP retransmit. */
>
> @@ -326,6 +327,7 @@ struct tcp_sock {
> * socket. Used to retransmit SYNACKs etc.
> */
> struct request_sock *fastopen_rsk;
> + u32 *saved_syn;
> };
>
> enum tsq_flags {
> @@ -393,4 +395,10 @@ static inline int fastopen_init_queue(struct sock *sk, int backlog)
> return 0;
> }
>
> +static inline void tcp_saved_syn_free(struct tcp_sock *tp)
> +{
> + kfree(tp->saved_syn);
> + tp->saved_syn = NULL;
> +}
> +
> #endif /* _LINUX_TCP_H */
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 9f4265ce8892..87935cad2f7b 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -64,6 +64,7 @@ struct request_sock {
> struct timer_list rsk_timer;
> const struct request_sock_ops *rsk_ops;
> struct sock *sk;
> + u32 *saved_syn;
> u32 secid;
> u32 peer_secid;
> };
> @@ -77,7 +78,7 @@ reqsk_alloc(const struct request_sock_ops *ops, struct sock *sk_listener)
> req->rsk_ops = ops;
> sock_hold(sk_listener);
> req->rsk_listener = sk_listener;
> -
> + req->saved_syn = NULL;
> /* Following is temporary. It is coupled with debugging
> * helpers in reqsk_put() & reqsk_free()
> */
> @@ -104,6 +105,7 @@ static inline void reqsk_free(struct request_sock *req)
> req->rsk_ops->destructor(req);
> if (req->rsk_listener)
> sock_put(req->rsk_listener);
> + kfree(req->saved_syn);
> kmem_cache_free(req->rsk_ops->slab, req);
> }
>
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index faa72f4fa547..51ebedba577f 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -113,6 +113,8 @@ enum {
> #define TCP_TIMESTAMP 24
> #define TCP_NOTSENT_LOWAT 25 /* limit number of unsent bytes in write queue */
> #define TCP_CC_INFO 26 /* Get Congestion Control (optional) info */
> +#define TCP_SAVE_SYN 27 /* Record SYN headers for new connections */
> +#define TCP_SAVED_SYN 28 /* Get SYN headers recorded for connection */
>
> struct tcp_repair_opt {
> __u32 opt_code;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 46efa03d2b11..ecccfdc50d76 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2482,6 +2482,13 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
> icsk->icsk_syn_retries = val;
> break;
>
> + case TCP_SAVE_SYN:
> + if (val < 0 || val > 1)
> + err = -EINVAL;
> + else
> + tp->save_syn = val;
> + break;
> +
> case TCP_LINGER2:
> if (val < 0)
> tp->linger2 = -1;
> @@ -2818,6 +2825,34 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
> case TCP_NOTSENT_LOWAT:
> val = tp->notsent_lowat;
> break;
> + case TCP_SAVE_SYN:
> + val = tp->save_syn;
> + break;
> + case TCP_SAVED_SYN: {
> + if (get_user(len, optlen))
> + return -EFAULT;
> +
> + lock_sock(sk);
> + if (tp->saved_syn) {
> + len = min_t(unsigned int, tp->saved_syn[0], len);
> + if (put_user(len, optlen)) {
> + release_sock(sk);
> + return -EFAULT;
> + }
> + if (copy_to_user(optval, tp->saved_syn + 1, len)) {
> + release_sock(sk);
> + return -EFAULT;
> + }
> + tcp_saved_syn_free(tp);
> + release_sock(sk);
> + } else {
> + release_sock(sk);
> + len = 0;
> + if (put_user(len, optlen))
> + return -EFAULT;
> + }
> + return 0;
> + }
> default:
> return -ENOPROTOOPT;
> }
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 09bdc4abfcbb..df2ca615cd0c 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6060,6 +6060,23 @@ static bool tcp_syn_flood_action(struct sock *sk,
> return want_cookie;
> }
>
> +static void tcp_reqsk_record_syn(const struct sock *sk,
> + struct request_sock *req,
> + const struct sk_buff *skb)
> +{
> + if (tcp_sk(sk)->save_syn) {
> + u32 len = skb_network_header_len(skb) + tcp_hdrlen(skb);
> + u32 *copy;
> +
> + copy = kmalloc(len + sizeof(u32), GFP_ATOMIC);
> + if (copy) {
> + copy[0] = len;
> + memcpy(©[1], skb_network_header(skb), len);
> + req->saved_syn = copy;
> + }
> + }
> +}
> +
> int tcp_conn_request(struct request_sock_ops *rsk_ops,
> const struct tcp_request_sock_ops *af_ops,
> struct sock *sk, struct sk_buff *skb)
> @@ -6192,6 +6209,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> tcp_rsk(req)->tfo_listener = false;
> af_ops->queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
> }
> + tcp_reqsk_record_syn(sk, req, skb);
>
> return 0;
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index fc1c658ec6c1..91cb4768a860 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1802,6 +1802,7 @@ void tcp_v4_destroy_sock(struct sock *sk)
>
> /* If socket is aborted during connect operation */
> tcp_free_fastopen_req(tp);
> + tcp_saved_syn_free(tp);
>
> sk_sockets_allocated_dec(sk);
> sock_release_memcg(sk);
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index e5d7649136fc..ebe2ab2596ed 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -536,6 +536,9 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
> newtp->fastopen_rsk = NULL;
> newtp->syn_data_acked = 0;
>
> + newtp->saved_syn = req->saved_syn;
> + req->saved_syn = NULL;
> +
> TCP_INC_STATS_BH(sock_net(sk), TCP_MIB_PASSIVEOPENS);
> }
> return newsk;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
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