* Re: [PATCH v5 09/45] smp, cpu hotplug: Fix smp_call_function_*() to prevent CPU offline properly
From: Srivatsa S. Bhat @ 2013-02-10 19:41 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130209000717.GP2666@linux.vnet.ibm.com>
On 02/09/2013 05:37 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:05:10PM +0530, Srivatsa S. Bhat wrote:
>> Once stop_machine() is gone from the CPU offline path, we won't be able to
>> depend on preempt_disable() to prevent CPUs from going offline from under us.
>>
>> Use the get/put_online_cpus_atomic() APIs to prevent CPUs from going offline,
>> while invoking from atomic context.
>>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>
> Would it make sense for get_online_cpus_atomic() to return the current
> CPU number?
Hmm, I'm not so sure. I tried to model it after get_online_cpus(), which doesn't
return anything (for other reasons, of course..)
Moreover, a function name like *_cpu_* returning the CPU number would be intuitive.
But a name such as *_cpus_* (plural) returning a CPU number might appear confusing..
And also I don't think it'll make a huge improvement in the callers.. (We might
be better off avoiding an smp_processor_id() if possible, since this function could
be called in very hot paths).. So I don't see a strong case for returning the
CPU number. But let me know if you think it'll still be worth it for some reason...
> Looks good otherwise.
>
Thank you very much for the detailed review, Paul!
Regards,
Srivatsa S. Bhat
>
>> ---
>>
>> kernel/smp.c | 40 ++++++++++++++++++++++++++--------------
>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index 29dd40a..f421bcc 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -310,7 +310,8 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>> * prevent preemption and reschedule on another processor,
>> * as well as CPU removal
>> */
>> - this_cpu = get_cpu();
>> + get_online_cpus_atomic();
>> + this_cpu = smp_processor_id();
>>
>> /*
>> * Can deadlock when called with interrupts disabled.
>> @@ -342,7 +343,7 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
>> }
>> }
>>
>> - put_cpu();
>> + put_online_cpus_atomic();
>>
>> return err;
>> }
>> @@ -371,8 +372,10 @@ int smp_call_function_any(const struct cpumask *mask,
>> const struct cpumask *nodemask;
>> int ret;
>>
>> + get_online_cpus_atomic();
>> /* Try for same CPU (cheapest) */
>> - cpu = get_cpu();
>> + cpu = smp_processor_id();
>> +
>> if (cpumask_test_cpu(cpu, mask))
>> goto call;
>>
>> @@ -388,7 +391,7 @@ int smp_call_function_any(const struct cpumask *mask,
>> cpu = cpumask_any_and(mask, cpu_online_mask);
>> call:
>> ret = smp_call_function_single(cpu, func, info, wait);
>> - put_cpu();
>> + put_online_cpus_atomic();
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(smp_call_function_any);
>> @@ -409,25 +412,28 @@ void __smp_call_function_single(int cpu, struct call_single_data *data,
>> unsigned int this_cpu;
>> unsigned long flags;
>>
>> - this_cpu = get_cpu();
>> + get_online_cpus_atomic();
>> +
>> + this_cpu = smp_processor_id();
>> +
>> /*
>> * Can deadlock when called with interrupts disabled.
>> * We allow cpu's that are not yet online though, as no one else can
>> * send smp call function interrupt to this cpu and as such deadlocks
>> * can't happen.
>> */
>> - WARN_ON_ONCE(cpu_online(smp_processor_id()) && wait && irqs_disabled()
>> + WARN_ON_ONCE(cpu_online(this_cpu) && wait && irqs_disabled()
>> && !oops_in_progress);
>>
>> if (cpu == this_cpu) {
>> local_irq_save(flags);
>> data->func(data->info);
>> local_irq_restore(flags);
>> - } else {
>> + } else if ((unsigned)cpu < nr_cpu_ids && cpu_online(cpu)) {
>> csd_lock(data);
>> generic_exec_single(cpu, data, wait);
>> }
>> - put_cpu();
>> + put_online_cpus_atomic();
>> }
>>
>> /**
>> @@ -451,6 +457,8 @@ void smp_call_function_many(const struct cpumask *mask,
>> unsigned long flags;
>> int refs, cpu, next_cpu, this_cpu = smp_processor_id();
>>
>> + get_online_cpus_atomic();
>> +
>> /*
>> * Can deadlock when called with interrupts disabled.
>> * We allow cpu's that are not yet online though, as no one else can
>> @@ -467,17 +475,18 @@ void smp_call_function_many(const struct cpumask *mask,
>>
>> /* No online cpus? We're done. */
>> if (cpu >= nr_cpu_ids)
>> - return;
>> + goto out_unlock;
>>
>> /* Do we have another CPU which isn't us? */
>> next_cpu = cpumask_next_and(cpu, mask, cpu_online_mask);
>> if (next_cpu == this_cpu)
>> - next_cpu = cpumask_next_and(next_cpu, mask, cpu_online_mask);
>> + next_cpu = cpumask_next_and(next_cpu, mask,
>> + cpu_online_mask);
>>
>> /* Fastpath: do that cpu by itself. */
>> if (next_cpu >= nr_cpu_ids) {
>> smp_call_function_single(cpu, func, info, wait);
>> - return;
>> + goto out_unlock;
>> }
>>
>> data = &__get_cpu_var(cfd_data);
>> @@ -523,7 +532,7 @@ void smp_call_function_many(const struct cpumask *mask,
>> /* Some callers race with other cpus changing the passed mask */
>> if (unlikely(!refs)) {
>> csd_unlock(&data->csd);
>> - return;
>> + goto out_unlock;
>> }
>>
>> raw_spin_lock_irqsave(&call_function.lock, flags);
>> @@ -554,6 +563,9 @@ void smp_call_function_many(const struct cpumask *mask,
>> /* Optionally wait for the CPUs to complete */
>> if (wait)
>> csd_lock_wait(&data->csd);
>> +
>> +out_unlock:
>> + put_online_cpus_atomic();
>> }
>> EXPORT_SYMBOL(smp_call_function_many);
>>
>> @@ -574,9 +586,9 @@ EXPORT_SYMBOL(smp_call_function_many);
>> */
>> int smp_call_function(smp_call_func_t func, void *info, int wait)
>> {
>> - preempt_disable();
>> + get_online_cpus_atomic();
>> smp_call_function_many(cpu_online_mask, func, info, wait);
>> - preempt_enable();
>> + put_online_cpus_atomic();
>>
>> return 0;
>> }
>>
^ permalink raw reply
* Re: [PATCH v5 07/45] CPU hotplug: Provide APIs to prevent CPU offline from atomic context
From: Srivatsa S. Bhat @ 2013-02-10 19:34 UTC (permalink / raw)
To: Namhyung Kim
Cc: linux-doc, peterz, fweisbec, linux-kernel, mingo, linux-arch,
linux, xiaoguangrong, wangyun, paulmck, nikunj, linux-pm, rusty,
rostedt, rjw, tglx, linux-arm-kernel, netdev, oleg, sbw, tj, akpm,
linuxppc-dev
In-Reply-To: <87mwvsuw68.fsf@sejong.aot.lge.com>
Hi Namhyung,
On 01/29/2013 03:51 PM, Namhyung Kim wrote:
> Hi Srivatsa,
>
> On Tue, 22 Jan 2013 13:04:54 +0530, Srivatsa S. Bhat wrote:
>> @@ -246,15 +291,21 @@ struct take_cpu_down_param {
>> static int __ref take_cpu_down(void *_param)
>> {
>> struct take_cpu_down_param *param = _param;
>> - int err;
>> + unsigned long flags;
>> + int err = 0;
>
> It seems no need to set 'err' to 0.
>
Sorry for the late reply. This mail got buried in my inbox and
I hadn't noticed it until now.. :-(
I'll remove the unnecessary initialization. Thank you!
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 06/45] percpu_rwlock: Allow writers to be readers, and add lockdep annotations
From: Srivatsa S. Bhat @ 2013-02-10 19:32 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130208234754.GM2666@linux.vnet.ibm.com>
On 02/09/2013 05:17 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:04:23PM +0530, Srivatsa S. Bhat wrote:
>> CPU hotplug (which will be the first user of per-CPU rwlocks) has a special
>> requirement with respect to locking: the writer, after acquiring the per-CPU
>> rwlock for write, must be allowed to take the same lock for read, without
>> deadlocking and without getting complaints from lockdep. In comparison, this
>> is similar to what get_online_cpus()/put_online_cpus() does today: it allows
>> a hotplug writer (who holds the cpu_hotplug.lock mutex) to invoke it without
>> locking issues, because it silently returns if the caller is the hotplug
>> writer itself.
>>
>> This can be easily achieved with per-CPU rwlocks as well (even without a
>> "is this a writer?" check) by incrementing the per-CPU refcount of the writer
>> immediately after taking the global rwlock for write, and then decrementing
>> the per-CPU refcount before releasing the global rwlock.
>> This ensures that any reader that comes along on that CPU while the writer is
>> active (on that same CPU), notices the non-zero value of the nested counter
>> and assumes that it is a nested read-side critical section and proceeds by
>> just incrementing the refcount. Thus we prevent the reader from taking the
>> global rwlock for read, which prevents the writer from deadlocking itself.
>>
>> Add that support and teach lockdep about this special locking scheme so
>> that it knows that this sort of usage is valid. Also add the required lockdep
>> annotations to enable it to detect common locking problems with per-CPU
>> rwlocks.
>
> Very nice! The write-side interrupt disabling ensures that the task
> stays on CPU, as required.
>
> One request: Could we please have a comment explaining the reasons for
> the writer incrementing and decrementing the reader reference count?
>
> It looked really really strange to me until I came back and read the
> commit log. ;-)
>
Sure :-)
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 05/45] percpu_rwlock: Make percpu-rwlocks IRQ-safe, optimally
From: Srivatsa S. Bhat @ 2013-02-10 19:30 UTC (permalink / raw)
To: Oleg Nesterov
Cc: tglx, peterz, tj, paulmck, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130210184209.GA3041@redhat.com>
On 02/11/2013 12:12 AM, Oleg Nesterov wrote:
> only one cosmetic nit...
>
> On 01/22, Srivatsa S. Bhat wrote:
>>
>> +#define READER_PRESENT (1UL << 16)
>> +#define READER_REFCNT_MASK (READER_PRESENT - 1)
>> +
>> #define reader_uses_percpu_refcnt(pcpu_rwlock, cpu) \
>> (ACCESS_ONCE(per_cpu(*((pcpu_rwlock)->reader_refcnt), cpu)))
>>
>> #define reader_nested_percpu(pcpu_rwlock) \
>> - (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) > 1)
>> + (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) & READER_REFCNT_MASK)
>>
>> #define writer_active(pcpu_rwlock) \
>> (__this_cpu_read(*((pcpu_rwlock)->writer_signal)))
>
> I think this all can go to lib/percpu-rwlock.c. Nobody needs to know
> these implementation details.
>
Ok, will move them.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 05/45] percpu_rwlock: Make percpu-rwlocks IRQ-safe, optimally
From: Srivatsa S. Bhat @ 2013-02-10 19:27 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130208234403.GL2666@linux.vnet.ibm.com>
On 02/09/2013 05:14 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:04:11PM +0530, Srivatsa S. Bhat wrote:
>> If interrupt handlers can also be readers, then one of the ways to make
>> per-CPU rwlocks safe, is to disable interrupts at the reader side before
>> trying to acquire the per-CPU rwlock and keep it disabled throughout the
>> duration of the read-side critical section.
[...]
>> -void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock)
>> +void percpu_read_lock_irqsafe(struct percpu_rwlock *pcpu_rwlock)
>> {
>> preempt_disable();
>>
>> /* First and foremost, let the writer know that a reader is active */
>> - this_cpu_inc(*pcpu_rwlock->reader_refcnt);
>> + this_cpu_add(*pcpu_rwlock->reader_refcnt, READER_PRESENT);
>>
>> /*
>> * If we are already using per-cpu refcounts, it is not safe to switch
>> * the synchronization scheme. So continue using the refcounts.
>> */
>> if (reader_nested_percpu(pcpu_rwlock)) {
>> - goto out;
>> + this_cpu_inc(*pcpu_rwlock->reader_refcnt);
>
> Hmmm... If the reader is nested, it -doesn't- need the memory barrier at
> the end of this function. If there is lots of nesting, it might be
> worth getting rid of it.
>
Yes, good point! Will get rid of it.
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 19:24 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Paul E. McKenney, tglx, peterz, tj, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <20130210180607.GA1375@redhat.com>
On 02/10/2013 11:36 PM, Oleg Nesterov wrote:
> On 02/08, Paul E. McKenney wrote:
>>
>> On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
>>>
>>> void percpu_read_unlock(struct percpu_rwlock *pcpu_rwlock)
>>> {
>>> - read_unlock(&pcpu_rwlock->global_rwlock);
>>
>> We need an smp_mb() here to keep the critical section ordered before the
>> this_cpu_dec() below. Otherwise, if a writer shows up just after we
>> exit the fastpath, that writer is not guaranteed to see the effects of
>> our critical section. Equivalently, the prior read-side critical section
>> just might see some of the writer's updates, which could be a bit of
>> a surprise to the reader.
>
> Agreed, we should not assume that a "reader" doesn't write. And we should
> ensure that this "read" section actually completes before this_cpu_dec().
>
Right, will fix.
>>> + /*
>>> + * We never allow heterogeneous nesting of readers. So it is trivial
>>> + * to find out the kind of reader we are, and undo the operation
>>> + * done by our corresponding percpu_read_lock().
>>> + */
>>> + if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
>>> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
>>> + smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
>>
>> Given an smp_mb() above, I don't understand the need for this smp_wmb().
>> Isn't the idea that if the writer sees ->reader_refcnt decremented to
>> zero, it also needs to see the effects of the corresponding reader's
>> critical section?
>
> I am equally confused ;)
>
> OTOH, we can probably aboid any barrier if reader_nested_percpu() == T.
>
Good point! Will add that optimization, thank you!
>
>>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>>> +{
>>> + unsigned int cpu;
>>> +
>>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>>
>> Why do we drop ourselves twice? More to the point, why is it important to
>> drop ourselves first?
>
> And don't we need mb() _before_ we clear ->writer_signal ?
>
Oh, right! Or, how about moving announce_writer_inactive() to _after_
write_unlock()?
>>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
>>> + unsigned int cpu)
>>> +{
>>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
>>
>> As I understand it, the purpose of this memory barrier is to ensure
>> that the stores in drop_writer_signal() happen before the reads from
>> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
>> race between a new reader attempting to use the fastpath and this writer
>> acquiring the lock. Unless I am confused, this must be smp_mb() rather
>> than smp_rmb().
>
> And note that before sync_reader() we call announce_writer_active() which
> already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> unneeded.
>
My intention was to help the writer see the ->reader_refcnt drop to zero
ASAP; hence I used smp_wmb() at reader and smp_rmb() here at the writer.
Please correct me if my understanding of memory barriers is wrong here..
> But, at the same time, could you confirm that we do not need another mb()
> after sync_all_readers() in percpu_write_lock() ? I mean, without mb(),
> can't this reader_uses_percpu_refcnt() LOAD leak into the critical section
> protected by ->global_rwlock? Then this LOAD can be re-ordered with other
> memory operations done by the writer.
>
Hmm.. it appears that we need a smp_mb() there.
>
>
> Srivatsa, I think that the code would be more understandable if you kill
> the helpers like sync_reader/raise_writer_signal. Perhaps even all "write"
> helpers, I am not sure. At least, it seems to me that all barriers should
> be moved to percpu_write_lock/unlock. But I won't insist of course, up to
> you.
>
Sure, sure. Even Tejun pointed out that those helpers are getting in the way
of readability. I'll get rid of them in the next version.
> And cosmetic nit... How about
>
> struct xxx {
> unsigned long reader_refcnt;
> bool writer_signal;
> }
>
> struct percpu_rwlock {
> struct xxx __percpu *xxx;
> rwlock_t global_rwlock;
> };
>
> ?
>
> This saves one alloc_percpu() and ensures that reader_refcnt/writer_signal
> are always in the same cache-line.
>
Ok, that sounds better. Will make that change. Thanks a lot Oleg!
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 19:10 UTC (permalink / raw)
To: paulmck
Cc: tglx, peterz, tj, oleg, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130208231017.GK2666@linux.vnet.ibm.com>
On 02/09/2013 04:40 AM, Paul E. McKenney wrote:
> On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
>> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
>> lock-ordering related problems (unlike per-cpu locks). However, global
>> rwlocks lead to unnecessary cache-line bouncing even when there are no
>> writers present, which can slow down the system needlessly.
>>
[...]
>
> Looks pretty close! Some comments interspersed below. Please either
> fix the code or my confusion, as the case may be. ;-)
>
Sure :-)
>> ---
>>
>> include/linux/percpu-rwlock.h | 10 +++
>> lib/percpu-rwlock.c | 128 ++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 136 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/percpu-rwlock.h b/include/linux/percpu-rwlock.h
>> index 8dec8fe..6819bb8 100644
>> --- a/include/linux/percpu-rwlock.h
>> +++ b/include/linux/percpu-rwlock.h
>> @@ -68,4 +68,14 @@ extern void percpu_free_rwlock(struct percpu_rwlock *);
>> __percpu_init_rwlock(pcpu_rwlock, #pcpu_rwlock, &rwlock_key); \
>> })
>>
>> +#define reader_uses_percpu_refcnt(pcpu_rwlock, cpu) \
>> + (ACCESS_ONCE(per_cpu(*((pcpu_rwlock)->reader_refcnt), cpu)))
>> +
>> +#define reader_nested_percpu(pcpu_rwlock) \
>> + (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) > 1)
>> +
>> +#define writer_active(pcpu_rwlock) \
>> + (__this_cpu_read(*((pcpu_rwlock)->writer_signal)))
>> +
>> #endif
>> +
>> diff --git a/lib/percpu-rwlock.c b/lib/percpu-rwlock.c
>> index 80dad93..992da5c 100644
>> --- a/lib/percpu-rwlock.c
>> +++ b/lib/percpu-rwlock.c
>> @@ -64,21 +64,145 @@ void percpu_free_rwlock(struct percpu_rwlock *pcpu_rwlock)
>>
>> void percpu_read_lock(struct percpu_rwlock *pcpu_rwlock)
>> {
>> - read_lock(&pcpu_rwlock->global_rwlock);
>> + preempt_disable();
>> +
>> + /* First and foremost, let the writer know that a reader is active */
>> + this_cpu_inc(*pcpu_rwlock->reader_refcnt);
>> +
>> + /*
>> + * If we are already using per-cpu refcounts, it is not safe to switch
>> + * the synchronization scheme. So continue using the refcounts.
>> + */
>> + if (reader_nested_percpu(pcpu_rwlock)) {
>> + goto out;
>> + } else {
>> + /*
>> + * The write to 'reader_refcnt' must be visible before we
>> + * read 'writer_signal'.
>> + */
>> + smp_mb(); /* Paired with smp_rmb() in sync_reader() */
>> +
>> + if (likely(!writer_active(pcpu_rwlock))) {
>> + goto out;
>> + } else {
>> + /* Writer is active, so switch to global rwlock. */
>> + read_lock(&pcpu_rwlock->global_rwlock);
>> +
>> + /*
>> + * We might have raced with a writer going inactive
>> + * before we took the read-lock. So re-evaluate whether
>> + * we still need to hold the rwlock or if we can switch
>> + * back to per-cpu refcounts. (This also helps avoid
>> + * heterogeneous nesting of readers).
>> + */
>> + if (writer_active(pcpu_rwlock))
>
> The above writer_active() can be reordered with the following this_cpu_dec(),
> strange though it might seem. But this is OK because holding the rwlock
> is conservative. But might be worth a comment.
>
Ok..
>> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
>> + else
>
> In contrast, no reordering can happen here because read_unlock() is
> required to keep the critical section underneath the lock.
>
Ok..
>> + read_unlock(&pcpu_rwlock->global_rwlock);
>> + }
>> + }
>> +
>> +out:
>> + /* Prevent reordering of any subsequent reads */
>> + smp_rmb();
>
> This should be smp_mb(). "Readers" really can do writes. Hence the
> name lglock -- "local/global" rather than "reader/writer".
>
Ok!
[ We were trying to avoid full memory barriers in get/put_online_cpus_atomic()
in the fastpath, as far as possible... Now it feels like all that discussion
was for nothing :-( ]
>> }
>>
>> void percpu_read_unlock(struct percpu_rwlock *pcpu_rwlock)
>> {
>> - read_unlock(&pcpu_rwlock->global_rwlock);
>
> We need an smp_mb() here to keep the critical section ordered before the
> this_cpu_dec() below. Otherwise, if a writer shows up just after we
> exit the fastpath, that writer is not guaranteed to see the effects of
> our critical section. Equivalently, the prior read-side critical section
> just might see some of the writer's updates, which could be a bit of
> a surprise to the reader.
>
Ok, will add it.
>> + /*
>> + * We never allow heterogeneous nesting of readers. So it is trivial
>> + * to find out the kind of reader we are, and undo the operation
>> + * done by our corresponding percpu_read_lock().
>> + */
>> + if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
>> + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
>> + smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
>
> Given an smp_mb() above, I don't understand the need for this smp_wmb().
> Isn't the idea that if the writer sees ->reader_refcnt decremented to
> zero, it also needs to see the effects of the corresponding reader's
> critical section?
>
Not sure what you meant, but my idea here was that the writer should see
the reader_refcnt falling to zero as soon as possible, to avoid keeping the
writer waiting in a tight loop for longer than necessary.
I might have been a little over-zealous to use lighter memory barriers though,
(given our lengthy discussions in the previous versions to reduce the memory
barrier overheads), so the smp_wmb() used above might be wrong.
So, are you saying that the smp_mb() you indicated above would be enough
to make the writer observe the 1->0 transition of reader_refcnt immediately?
> Or am I missing something subtle here? In any case, if this smp_wmb()
> really is needed, there should be some subsequent write that the writer
> might observe. From what I can see, there is no subsequent write from
> this reader that the writer cares about.
>
I thought the smp_wmb() here and the smp_rmb() at the writer would ensure
immediate reflection of the reader state at the writer side... Please correct
me if my understanding is incorrect.
>> + } else {
>> + read_unlock(&pcpu_rwlock->global_rwlock);
>> + }
>> +
>> + preempt_enable();
>> +}
>> +
>> +static inline void raise_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>> + unsigned int cpu)
>> +{
>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = true;
>> +}
>> +
>> +static inline void drop_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>> + unsigned int cpu)
>> +{
>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = false;
>> +}
>> +
>> +static void announce_writer_active(struct percpu_rwlock *pcpu_rwlock)
>> +{
>> + unsigned int cpu;
>> +
>> + for_each_online_cpu(cpu)
>> + raise_writer_signal(pcpu_rwlock, cpu);
>> +
>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>> +}
>> +
>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>> +{
>> + unsigned int cpu;
>> +
>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>
> Why do we drop ourselves twice? More to the point, why is it important to
> drop ourselves first?
>
I don't see where we are dropping ourselves twice. Note that we are no longer
in the cpu_online_mask, so the 'for' loop below won't include us. So we need
to manually drop ourselves. It doesn't matter whether we drop ourselves first
or later.
>> +
>> + for_each_online_cpu(cpu)
>> + drop_writer_signal(pcpu_rwlock, cpu);
>> +
>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>> +}
>> +
>> +/*
>> + * Wait for the reader to see the writer's signal and switch from percpu
>> + * refcounts to global rwlock.
>> + *
>> + * If the reader is still using percpu refcounts, wait for him to switch.
>> + * Else, we can safely go ahead, because either the reader has already
>> + * switched over, or the next reader that comes along on that CPU will
>> + * notice the writer's signal and will switch over to the rwlock.
>> + */
>> +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
>> + unsigned int cpu)
>> +{
>> + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
>
> As I understand it, the purpose of this memory barrier is to ensure
> that the stores in drop_writer_signal() happen before the reads from
> ->reader_refcnt in reader_uses_percpu_refcnt(),
No, that was not what I intended. announce_writer_inactive() already does
a full smp_mb() after calling drop_writer_signal().
I put the smp_rmb() here and the smp_wmb() at the reader side (after updates
to the ->reader_refcnt) to reflect the state change of ->reader_refcnt
immediately at the writer, so that the writer doesn't have to keep spinning
unnecessarily still referring to the old (non-zero) value of ->reader_refcnt.
Or perhaps I am confused about how to use memory barriers properly.. :-(
> thus preventing the
> race between a new reader attempting to use the fastpath and this writer
> acquiring the lock. Unless I am confused, this must be smp_mb() rather
> than smp_rmb().
>
> Also, why not just have a single smp_mb() at the beginning of
> sync_all_readers() instead of executing one barrier per CPU?
Well, since my intention was to help the writer see the update (->reader_refcnt
dropping to zero) ASAP, I kept the multiple smp_rmb()s.
>
>> +
>> + while (reader_uses_percpu_refcnt(pcpu_rwlock, cpu))
>> + cpu_relax();
>> +}
>> +
>> +static void sync_all_readers(struct percpu_rwlock *pcpu_rwlock)
>> +{
>> + unsigned int cpu;
>> +
>> + for_each_online_cpu(cpu)
>> + sync_reader(pcpu_rwlock, cpu);
>> }
>>
>> void percpu_write_lock(struct percpu_rwlock *pcpu_rwlock)
>> {
>> + /*
>> + * Tell all readers that a writer is becoming active, so that they
>> + * start switching over to the global rwlock.
>> + */
>> + announce_writer_active(pcpu_rwlock);
>> + sync_all_readers(pcpu_rwlock);
>> write_lock(&pcpu_rwlock->global_rwlock);
>> }
>>
>> void percpu_write_unlock(struct percpu_rwlock *pcpu_rwlock)
>> {
>> + /*
>> + * Inform all readers that we are done, so that they can switch back
>> + * to their per-cpu refcounts. (We don't need to wait for them to
>> + * see it).
>> + */
>> + announce_writer_inactive(pcpu_rwlock);
>> write_unlock(&pcpu_rwlock->global_rwlock);
>> }
>>
>>
Thanks a lot for your detailed review and comments! :-)
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [IPv6] interface-local multicast escapes the local node
From: Hannes Frederic Sowa @ 2013-02-10 18:49 UTC (permalink / raw)
To: YOSHIFUJI Hideaki; +Cc: Erik Hugne, netdev
In-Reply-To: <5117EA2D.1020106@linux-ipv6.org>
On Mon, Feb 11, 2013 at 03:42:53AM +0900, YOSHIFUJI Hideaki wrote:
> Hannes Frederic Sowa wrote:
> > I was looking at getpeername et. al. where we should report the scope
> > back to the user. A common pattern is:
> >
> > if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
> > sin->sin6_scope_id = IP6CB(skb)->iif;
> >
> > I propose to introduce something like 'bool ipv6_addr_intf_scoped(in6_addr)'
> > and let it check for ll addresses and interface scoped addresses.
>
> Hmm, maybe, we might want to say:
>
> __u32 __ipv6_iface_scope_id(int type, unsigned int iface)
> {
> if (type == IPV6_ADDR_ANY ||
> type & IPV6_ADDR_LOOPBACK ||
> __ipv6_addr_src_scope(type) > IPV6_ADDR_SCOPE_LINKLOCAL)
> return 0;
> return iface;
> }
>
> __u32 ipv6_iface_scope_id(const struct in6_addr *addr, unsigned int iface)
> {
> return __ipv6_iface_scope_id(__ipv6_addr_type(addr), iface);
> }
>
> And then,
> sin->sin6_scope_id = __ipv6_iface_scope_id(__ipv6_addr_type(&sin->sin6_addr),
> IP6CB(skb)->iif);
> or
> sin->sin6_scope_id = ipv6_iface_scope_id(&sin->sin6_addr, IP6CB(skb)->iif);
I like it. Will try to convert some checks and let you know how it turned out.
^ permalink raw reply
* Re: [IPv6] interface-local multicast escapes the local node
From: YOSHIFUJI Hideaki @ 2013-02-10 18:42 UTC (permalink / raw)
To: hannes; +Cc: Erik Hugne, netdev, YOSHIFUJI Hideaki
In-Reply-To: <20130210153230.GA21377@order.stressinduktion.org>
Hannes Frederic Sowa wrote:
> On Sat, Feb 09, 2013 at 11:12:46PM +0900, YOSHIFUJI Hideaki wrote:
>> Hi,
>>
>> Hannes Frederic Sowa wrote:
>>> On Wed, Feb 06, 2013 at 05:54:15PM +0100, Hannes Frederic Sowa wrote:
>>>> On Thu, Feb 07, 2013 at 12:24:14AM +0900, YOSHIFUJI Hideaki wrote:
>>>>> NAK. I think we should select routes via loopback device here.
>>>>
>>>> Will try your idea, thanks.
>>>
>>> Does this patch look reasonable? Btw. i am pleased to see this kind of
>>> things work out as expected most of the time (addrtype checking etc. all
>>> in place). :)
>>>
>>
>> Well, I rethink of what "interface-local" means.
>>
>> It seems applications will join ff01::/16%eth0 instead of ff01::/16%lo.
>> If so, your original patch seems better. My bad, sorry.
>
> I was looking at getpeername et. al. where we should report the scope
> back to the user. A common pattern is:
>
> if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
> sin->sin6_scope_id = IP6CB(skb)->iif;
>
> I propose to introduce something like 'bool ipv6_addr_intf_scoped(in6_addr)'
> and let it check for ll addresses and interface scoped addresses.
Hmm, maybe, we might want to say:
__u32 __ipv6_iface_scope_id(int type, unsigned int iface)
{
if (type == IPV6_ADDR_ANY ||
type & IPV6_ADDR_LOOPBACK ||
__ipv6_addr_src_scope(type) > IPV6_ADDR_SCOPE_LINKLOCAL)
return 0;
return iface;
}
__u32 ipv6_iface_scope_id(const struct in6_addr *addr, unsigned int iface)
{
return __ipv6_iface_scope_id(__ipv6_addr_type(addr), iface);
}
And then,
sin->sin6_scope_id = __ipv6_iface_scope_id(__ipv6_addr_type(&sin->sin6_addr),
IP6CB(skb)->iif);
or
sin->sin6_scope_id = ipv6_iface_scope_id(&sin->sin6_addr, IP6CB(skb)->iif);
--yoshfuji
^ permalink raw reply
* Re: [PATCH v5 05/45] percpu_rwlock: Make percpu-rwlocks IRQ-safe, optimally
From: Oleg Nesterov @ 2013-02-10 18:42 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: tglx, peterz, tj, paulmck, rusty, mingo, akpm, namhyung, rostedt,
wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux, nikunj,
linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev, netdev,
linux-doc, linux-kernel
In-Reply-To: <20130122073400.13822.52336.stgit@srivatsabhat.in.ibm.com>
only one cosmetic nit...
On 01/22, Srivatsa S. Bhat wrote:
>
> +#define READER_PRESENT (1UL << 16)
> +#define READER_REFCNT_MASK (READER_PRESENT - 1)
> +
> #define reader_uses_percpu_refcnt(pcpu_rwlock, cpu) \
> (ACCESS_ONCE(per_cpu(*((pcpu_rwlock)->reader_refcnt), cpu)))
>
> #define reader_nested_percpu(pcpu_rwlock) \
> - (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) > 1)
> + (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) & READER_REFCNT_MASK)
>
> #define writer_active(pcpu_rwlock) \
> (__this_cpu_read(*((pcpu_rwlock)->writer_signal)))
I think this all can go to lib/percpu-rwlock.c. Nobody needs to know
these implementation details.
Oleg.
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-02-10 18:38 UTC (permalink / raw)
To: paulmck
Cc: Namhyung Kim, Tejun Heo, tglx, peterz, oleg, rusty, mingo, akpm,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel, walken
In-Reply-To: <20130208224742.GJ2666@linux.vnet.ibm.com>
On 02/09/2013 04:17 AM, Paul E. McKenney wrote:
> On Tue, Jan 29, 2013 at 08:12:37PM +0900, Namhyung Kim wrote:
>> On Thu, 24 Jan 2013 10:00:04 +0530, Srivatsa S. Bhat wrote:
>>> On 01/24/2013 01:27 AM, Tejun Heo wrote:
>>>> On Thu, Jan 24, 2013 at 01:03:52AM +0530, Srivatsa S. Bhat wrote:
>>>>> CPU 0 CPU 1
>>>>>
>>>>> read_lock(&rwlock)
>>>>>
>>>>> write_lock(&rwlock) //spins, because CPU 0
>>>>> //has acquired the lock for read
>>>>>
>>>>> read_lock(&rwlock)
>>>>> ^^^^^
>>>>> What happens here? Does CPU 0 start spinning (and hence deadlock) or will
>>>>> it continue realizing that it already holds the rwlock for read?
>>>>
>>>> I don't think rwlock allows nesting write lock inside read lock.
>>>> read_lock(); write_lock() will always deadlock.
>>>>
>>>
>>> Sure, I understand that :-) My question was, what happens when *two* CPUs
>>> are involved, as in, the read_lock() is invoked only on CPU 0 whereas the
>>> write_lock() is invoked on CPU 1.
>>>
>>> For example, the same scenario shown above, but with slightly different
>>> timing, will NOT result in a deadlock:
>>>
>>> Scenario 2:
>>> CPU 0 CPU 1
>>>
>>> read_lock(&rwlock)
>>>
>>>
>>> read_lock(&rwlock) //doesn't spin
>>>
>>> write_lock(&rwlock) //spins, because CPU 0
>>> //has acquired the lock for read
>>>
>>>
>>> So I was wondering whether the "fairness" logic of rwlocks would cause
>>> the second read_lock() to spin (in the first scenario shown above) because
>>> a writer is already waiting (and hence new readers should spin) and thus
>>> cause a deadlock.
>>
>> In my understanding, current x86 rwlock does basically this (of course,
>> in an atomic fashion):
>>
>>
>> #define RW_LOCK_BIAS 0x10000
>>
>> rwlock_init(rwlock)
>> {
>> rwlock->lock = RW_LOCK_BIAS;
>> }
>>
>> arch_read_lock(rwlock)
>> {
>> retry:
>> if (--rwlock->lock >= 0)
>> return;
>>
>> rwlock->lock++;
>> while (rwlock->lock < 1)
>> continue;
>>
>> goto retry;
>> }
>>
>> arch_write_lock(rwlock)
>> {
>> retry:
>> if ((rwlock->lock -= RW_LOCK_BIAS) == 0)
>> return;
>>
>> rwlock->lock += RW_LOCK_BIAS;
>> while (rwlock->lock != RW_LOCK_BIAS)
>> continue;
>>
>> goto retry;
>> }
>>
>>
>> So I can't find where the 'fairness' logic comes from..
>
> I looked through several of the rwlock implementations, and in all of
> them the writer backs off if it sees readers -- or refrains from asserting
> ownership of the lock to begin with.
>
> So it should be OK to use rwlock as shown in the underlying patch.
>
Thanks a lot for confirming that Paul! So I guess we can use rwlocks
as it is, since its behaviour suits our needs perfectly. So I won't tinker
with atomic counters for a while, atleast not until someone starts making
rwlocks fair.. ;-)
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: [patch net-next v3 08/11] tbf: fix value set for q->ptokens
From: Jiri Pirko @ 2013-02-10 18:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <1360519208.20362.19.camel@edumazet-glaptop>
Sun, Feb 10, 2013 at 07:00:08PM CET, eric.dumazet@gmail.com wrote:
>On Sun, 2013-02-10 at 09:51 +0100, Jiri Pirko wrote:
>> Sun, Feb 10, 2013 at 09:21:38AM CET, eric.dumazet@gmail.com wrote:
>> >On Sun, 2013-02-10 at 09:18 +0100, Jiri Pirko wrote:
>> >> Sun, Feb 10, 2013 at 02:30:46AM CET, eric.dumazet@gmail.com wrote:
>> >> >On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote:
>> >> >> q->ptokens is in ns and we are assigning q->mtu directly to it. That is
>> >> >> wrong. psched_l2t_ns() should be used to compute correct value.
>> >> >>
>> >> >
>> >> >
>> >> >Not clear why its a separate patch, and not folded in the 6th
>> >>
>> >> This is independent on 6th. Would be needed even if 6th wouldn't be
>> >> there.
>> >
>> >When was this bug introduced then ?
>>
>> This has been present since the beginning of git. I'm unable to find bk
>> repo right now to tell you bk commit :/
>
>Current code doesnt store ptokens in ns, your patch is very confusing.
>
>q->mtu is properly setup by tc command.
Now I get it. q->mtu is not bytes but time instead. I will fix my patches.
>
>Its not dev->mtu at all.
>
>I claim you fix a bug added in 6th patch.
>
>It makes no sense to me to introduce a bug in 6th and fix it in 8th.
>
>
>
^ permalink raw reply
* Re: [PATCH v2 net-next] skbuff: create skb_panic() to let caller specify panic
From: Jiri Pirko @ 2013-02-10 18:24 UTC (permalink / raw)
To: Jean Sacren; +Cc: netdev
In-Reply-To: <1360484610-11432-1-git-send-email-sakiwit@gmail.com>
Sun, Feb 10, 2013 at 09:23:30AM CET, sakiwit@gmail.com wrote:
>Combine skb_over_panic() and skb_under_panic() into skb_panic() and let
>the caller specify whether it is skb_over_panic or skb_under_panic.
>
>In skb_panic() definition, change 'int sz' and 'here' to 'unsigned int
>size' and 'addr' for clarity, and accommodate the output message.
>Rewrite kernel doc for skb_panic().
>
>Signed-off-by: Jean Sacren <sakiwit@gmail.com>
>---
>v2: don't split format over multiple lines as advised by Joe Perches.
>
> net/core/skbuff.c | 49 +++++++++++++++----------------------------------
> 1 file changed, 15 insertions(+), 34 deletions(-)
>
>diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>index 6114c11..86d915b 100644
>--- a/net/core/skbuff.c
>+++ b/net/core/skbuff.c
>@@ -104,48 +104,27 @@ static const struct pipe_buf_operations sock_pipe_buf_ops = {
> .get = sock_pipe_buf_get,
> };
>
>-/*
>- * Keep out-of-line to prevent kernel bloat.
>- * __builtin_return_address is not used because it is not always
>- * reliable.
>- */
>-
> /**
>- * skb_over_panic - private function
>- * @skb: buffer
>- * @sz: size
>- * @here: address
>+ * skb_panic - private function for out-of-line support
>+ * @skb: buffer
>+ * @size: size
>+ * @addr: address
>+ * @panic: skb_over_panic or skb_under_panic
> *
>- * Out of line support code for skb_put(). Not user callable.
>- */
>-static void skb_over_panic(struct sk_buff *skb, int sz, void *here)
>-{
>- pr_emerg("%s: text:%p len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n",
>- __func__, here, skb->len, sz, skb->head, skb->data,
>- (unsigned long)skb->tail, (unsigned long)skb->end,
>- skb->dev ? skb->dev->name : "<NULL>");
>- BUG();
>-}
>-
>-/**
>- * skb_under_panic - private function
>- * @skb: buffer
>- * @sz: size
>- * @here: address
>- *
>- * Out of line support code for skb_push(). Not user callable.
>+ * Out-of-line support for skb_put() and skb_push(). Not user callable.
>+ * Keep out-of-line to prevent kernel bloat.
>+ * __builtin_return_address is not used because it is not always reliable.
> */
>-
>-static void skb_under_panic(struct sk_buff *skb, int sz, void *here)
>+static void skb_panic(struct sk_buff *skb, unsigned int size, void *addr,
>+ const char panic[])
> {
> pr_emerg("%s: text:%p len:%d put:%d head:%p data:%p tail:%#lx end:%#lx dev:%s\n",
>- __func__, here, skb->len, sz, skb->head, skb->data,
>+ panic, addr, skb->len, size, skb->head, skb->data,
> (unsigned long)skb->tail, (unsigned long)skb->end,
> skb->dev ? skb->dev->name : "<NULL>");
> BUG();
> }
>
>-
> /*
> * kmalloc_reserve is a wrapper around kmalloc_node_track_caller that tells
> * the caller if emergency pfmemalloc reserves are being used. If it is and
>@@ -1259,12 +1238,13 @@ EXPORT_SYMBOL(skb_pad);
> */
> unsigned char *skb_put(struct sk_buff *skb, unsigned int len)
> {
>+ const char panic[] = "skb_over_panic";
How about to leave skb_under_panic() and skb_over_panic() wrapping
functions there just to wrap "skb_panic(skb, sz, addr, __func__);" call?
Seems nice that to state the name in str...
> unsigned char *tmp = skb_tail_pointer(skb);
> SKB_LINEAR_ASSERT(skb);
> skb->tail += len;
> skb->len += len;
> if (unlikely(skb->tail > skb->end))
>- skb_over_panic(skb, len, __builtin_return_address(0));
>+ skb_panic(skb, len, __builtin_return_address(0), panic);
> return tmp;
> }
> EXPORT_SYMBOL(skb_put);
>@@ -1280,10 +1260,11 @@ EXPORT_SYMBOL(skb_put);
> */
> unsigned char *skb_push(struct sk_buff *skb, unsigned int len)
> {
>+ const char panic[] = "skb_under_panic";
> skb->data -= len;
> skb->len += len;
> if (unlikely(skb->data<skb->head))
>- skb_under_panic(skb, len, __builtin_return_address(0));
>+ skb_panic(skb, len, __builtin_return_address(0), panic);
> return skb->data;
> }
> EXPORT_SYMBOL(skb_push);
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] ipv6: don't accept multicast traffic with scop 0
From: YOSHIFUJI Hideaki @ 2013-02-10 18:09 UTC (permalink / raw)
To: netdev, David Miller; +Cc: YOSHIFUJI Hideaki
In-Reply-To: <20130210153522.GA20897@order.stressinduktion.org>
Hannes Frederic Sowa wrote:
> v2:
> a) moved before multicast source address check
> b) changed comment to netdev style
>
> Cc: Erik Hugne <erik.hugne@ericsson.com>
> Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
> net/ipv6/ip6_input.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index 4ac5bf3..521d9fd 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -118,6 +118,15 @@ int ipv6_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt
> ipv6_addr_loopback(&hdr->daddr))
> goto err;
>
> + /* RFC4291 2.7
> + * Nodes must not originate a packet to a multicast address whose scop
> + * field contains the reserved value 0; if such a packet is received, it
> + * must be silently dropped.
> + */
> + if (ipv6_addr_is_multicast(&hdr->daddr) &&
> + IPV6_ADDR_MC_SCOPE(&hdr->daddr) == 0)
> + goto err;
> +
> /*
> * RFC4291 2.7
> * Multicast addresses must not be used as source addresses in IPv6
>
Acked-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
--yoshfuji
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Oleg Nesterov @ 2013-02-10 18:06 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Srivatsa S. Bhat, tglx, peterz, tj, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <20130208231017.GK2666@linux.vnet.ibm.com>
On 02/08, Paul E. McKenney wrote:
>
> On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
> >
> > void percpu_read_unlock(struct percpu_rwlock *pcpu_rwlock)
> > {
> > - read_unlock(&pcpu_rwlock->global_rwlock);
>
> We need an smp_mb() here to keep the critical section ordered before the
> this_cpu_dec() below. Otherwise, if a writer shows up just after we
> exit the fastpath, that writer is not guaranteed to see the effects of
> our critical section. Equivalently, the prior read-side critical section
> just might see some of the writer's updates, which could be a bit of
> a surprise to the reader.
Agreed, we should not assume that a "reader" doesn't write. And we should
ensure that this "read" section actually completes before this_cpu_dec().
> > + /*
> > + * We never allow heterogeneous nesting of readers. So it is trivial
> > + * to find out the kind of reader we are, and undo the operation
> > + * done by our corresponding percpu_read_lock().
> > + */
> > + if (__this_cpu_read(*pcpu_rwlock->reader_refcnt)) {
> > + this_cpu_dec(*pcpu_rwlock->reader_refcnt);
> > + smp_wmb(); /* Paired with smp_rmb() in sync_reader() */
>
> Given an smp_mb() above, I don't understand the need for this smp_wmb().
> Isn't the idea that if the writer sees ->reader_refcnt decremented to
> zero, it also needs to see the effects of the corresponding reader's
> critical section?
I am equally confused ;)
OTOH, we can probably aboid any barrier if reader_nested_percpu() == T.
> > +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> > +{
> > + unsigned int cpu;
> > +
> > + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>
> Why do we drop ourselves twice? More to the point, why is it important to
> drop ourselves first?
And don't we need mb() _before_ we clear ->writer_signal ?
> > +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> > + unsigned int cpu)
> > +{
> > + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
>
> As I understand it, the purpose of this memory barrier is to ensure
> that the stores in drop_writer_signal() happen before the reads from
> ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> race between a new reader attempting to use the fastpath and this writer
> acquiring the lock. Unless I am confused, this must be smp_mb() rather
> than smp_rmb().
And note that before sync_reader() we call announce_writer_active() which
already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
unneeded.
But, at the same time, could you confirm that we do not need another mb()
after sync_all_readers() in percpu_write_lock() ? I mean, without mb(),
can't this reader_uses_percpu_refcnt() LOAD leak into the critical section
protected by ->global_rwlock? Then this LOAD can be re-ordered with other
memory operations done by the writer.
Srivatsa, I think that the code would be more understandable if you kill
the helpers like sync_reader/raise_writer_signal. Perhaps even all "write"
helpers, I am not sure. At least, it seems to me that all barriers should
be moved to percpu_write_lock/unlock. But I won't insist of course, up to
you.
And cosmetic nit... How about
struct xxx {
unsigned long reader_refcnt;
bool writer_signal;
}
struct percpu_rwlock {
struct xxx __percpu *xxx;
rwlock_t global_rwlock;
};
?
This saves one alloc_percpu() and ensures that reader_refcnt/writer_signal
are always in the same cache-line.
Oleg.
^ permalink raw reply
* Re: [patch net-next v3 08/11] tbf: fix value set for q->ptokens
From: Eric Dumazet @ 2013-02-10 18:00 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <20130210085155.GB1570@minipsycho.orion>
On Sun, 2013-02-10 at 09:51 +0100, Jiri Pirko wrote:
> Sun, Feb 10, 2013 at 09:21:38AM CET, eric.dumazet@gmail.com wrote:
> >On Sun, 2013-02-10 at 09:18 +0100, Jiri Pirko wrote:
> >> Sun, Feb 10, 2013 at 02:30:46AM CET, eric.dumazet@gmail.com wrote:
> >> >On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote:
> >> >> q->ptokens is in ns and we are assigning q->mtu directly to it. That is
> >> >> wrong. psched_l2t_ns() should be used to compute correct value.
> >> >>
> >> >
> >> >
> >> >Not clear why its a separate patch, and not folded in the 6th
> >>
> >> This is independent on 6th. Would be needed even if 6th wouldn't be
> >> there.
> >
> >When was this bug introduced then ?
>
> This has been present since the beginning of git. I'm unable to find bk
> repo right now to tell you bk commit :/
Current code doesnt store ptokens in ns, your patch is very confusing.
q->mtu is properly setup by tc command.
Its not dev->mtu at all.
I claim you fix a bug added in 6th patch.
It makes no sense to me to introduce a bug in 6th and fix it in 8th.
^ permalink raw reply
* [RFC net-next (TAKE 3) 6/6] ipv6: IPv6 over Firewire (RFC3146) support.
From: YOSHIFUJI Hideaki @ 2013-02-10 17:47 UTC (permalink / raw)
To: netdev, linux1394-devel; +Cc: stephan.gatzka, davem, stefanr, yoshfuji
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
drivers/firewire/Kconfig | 6 ++--
drivers/firewire/net.c | 85 +++++++++++++++++++++++++++++++++++++++++-----
net/ipv6/addrconf.c | 20 ++++++++++-
3 files changed, 98 insertions(+), 13 deletions(-)
diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
index 7224533..ad8d16c 100644
--- a/drivers/firewire/Kconfig
+++ b/drivers/firewire/Kconfig
@@ -47,9 +47,9 @@ config FIREWIRE_NET
tristate "IP networking over 1394"
depends on FIREWIRE && INET
help
- This enables IPv4 over IEEE 1394, providing IP connectivity with
- other implementations of RFC 2734 as found on several operating
- systems. Multicast support is currently limited.
+ This enables IPv4/IPv6 over IEEE 1394, providing IP connectivity
+ with other implementations of RFC 2734/RFC3146 as found on
+ several operating systems. Multicast support is currently limited.
To compile this driver as a module, say M here: The module will be
called firewire-net.
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 177a092..a3c922d 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -1,5 +1,6 @@
/*
* IPv4 over IEEE 1394, per RFC 2734
+ * IPv6 over IEEE 1394, per RFC 3146
*
* Copyright (C) 2009 Jay Fenlason <fenlason@redhat.com>
*
@@ -46,6 +47,7 @@
#define IANA_SPECIFIER_ID 0x00005eU
#define RFC2734_SW_VERSION 0x000001U
+#define RFC3146_SW_VERSION 0x000002U
#define IEEE1394_GASP_HDR_SIZE 8
@@ -505,6 +507,9 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
switch (ether_type) {
case ETH_P_ARP:
case ETH_P_IP:
+#if IS_ENABLED(CONFIG_IPV6)
+ case ETH_P_IPV6:
+#endif
break;
default:
goto err;
@@ -768,11 +773,18 @@ static void fwnet_receive_broadcast(struct fw_iso_context *context,
ver = be32_to_cpu(buf_ptr[1]) & 0xffffff;
source_node_id = be32_to_cpu(buf_ptr[0]) >> 16;
- if (specifier_id == IANA_SPECIFIER_ID && ver == RFC2734_SW_VERSION) {
- buf_ptr += 2;
- length -= IEEE1394_GASP_HDR_SIZE;
- fwnet_incoming_packet(dev, buf_ptr, length, source_node_id,
- context->card->generation, true);
+ if (specifier_id == IANA_SPECIFIER_ID) {
+ switch (ver) {
+ case RFC2734_SW_VERSION:
+#if IS_ENABLED(CONFIG_IPV6)
+ case RFC3146_SW_VERSION:
+#endif
+ buf_ptr += 2;
+ length -= IEEE1394_GASP_HDR_SIZE;
+ fwnet_incoming_packet(dev, buf_ptr, length,
+ source_node_id,
+ context->card->generation, true);
+ }
}
packet.payload_length = dev->rcv_buffer_size;
@@ -979,8 +991,18 @@ static int fwnet_send_packet(struct fwnet_packet_task *ptask)
p = skb_push(ptask->skb, IEEE1394_GASP_HDR_SIZE);
put_unaligned_be32(node_id << 16 | IANA_SPECIFIER_ID >> 8, p);
- put_unaligned_be32((IANA_SPECIFIER_ID & 0xff) << 24
- | RFC2734_SW_VERSION, &p[4]);
+
+ switch (ptask->skb->protocol) {
+ default:
+ put_unaligned_be32((IANA_SPECIFIER_ID & 0xff) << 24
+ | RFC2734_SW_VERSION, &p[4]);
+ break;
+#if IS_ENABLED(CONFIG_IPV6)
+ case htons(ETH_P_IPV6):
+ put_unaligned_be32((IANA_SPECIFIER_ID & 0xff) << 24
+ | RFC3146_SW_VERSION, &p[4]);
+#endif
+ }
/* We should not transmit if broadcast_channel.valid == 0. */
fw_send_request(dev->card, &ptask->transaction,
@@ -1200,6 +1222,9 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net)
switch (proto) {
case htons(ETH_P_ARP):
case htons(ETH_P_IP):
+#if IS_ENABLED(CONFIG_IPV6)
+ case htons(ETH_P_IPV6):
+#endif
break;
default:
goto fail;
@@ -1545,6 +1570,14 @@ static const struct ieee1394_device_id fwnet_id_table[] = {
.specifier_id = IANA_SPECIFIER_ID,
.version = RFC2734_SW_VERSION,
},
+#if IS_ENABLED(CONFIG_IPV6)
+ {
+ .match_flags = IEEE1394_MATCH_SPECIFIER_ID |
+ IEEE1394_MATCH_VERSION,
+ .specifier_id = IANA_SPECIFIER_ID,
+ .version = RFC3146_SW_VERSION,
+ },
+#endif
{ }
};
@@ -1582,6 +1615,30 @@ static struct fw_descriptor rfc2374_unit_directory = {
.data = rfc2374_unit_directory_data
};
+#if IS_ENABLED(CONFIG_IPV6)
+static const u32 rfc3146_unit_directory_data[] = {
+ 0x00040000, /* directory_length */
+ 0x1200005e, /* unit_specifier_id: IANA */
+ 0x81000003, /* textual descriptor offset */
+ 0x13000002, /* unit_sw_version: RFC 3146 */
+ 0x81000005, /* textual descriptor offset */
+ 0x00030000, /* descriptor_length */
+ 0x00000000, /* text */
+ 0x00000000, /* minimal ASCII, en */
+ 0x49414e41, /* I A N A */
+ 0x00030000, /* descriptor_length */
+ 0x00000000, /* text */
+ 0x00000000, /* minimal ASCII, en */
+ 0x49507636, /* I P v 6 */
+};
+
+static struct fw_descriptor rfc3146_unit_directory = {
+ .length = ARRAY_SIZE(rfc3146_unit_directory_data),
+ .key = (CSR_DIRECTORY | CSR_UNIT) << 24,
+ .data = rfc3146_unit_directory_data
+};
+#endif
+
static int __init fwnet_init(void)
{
int err;
@@ -1590,11 +1647,17 @@ static int __init fwnet_init(void)
if (err)
return err;
+#if IS_ENABLED(CONFIG_IPV6)
+ err = fw_core_add_descriptor(&rfc3146_unit_directory);
+ if (err)
+ goto out;
+#endif
+
fwnet_packet_task_cache = kmem_cache_create("packet_task",
sizeof(struct fwnet_packet_task), 0, 0, NULL);
if (!fwnet_packet_task_cache) {
err = -ENOMEM;
- goto out;
+ goto out2;
}
err = driver_register(&fwnet_driver.driver);
@@ -1602,7 +1665,11 @@ static int __init fwnet_init(void)
return 0;
kmem_cache_destroy(fwnet_packet_task_cache);
+out2:
+#if IS_ENABLED(CONFIG_IPV6)
+ fw_core_remove_descriptor(&rfc3146_unit_directory);
out:
+#endif
fw_core_remove_descriptor(&rfc2374_unit_directory);
return err;
@@ -1618,6 +1685,6 @@ static void __exit fwnet_cleanup(void)
module_exit(fwnet_cleanup);
MODULE_AUTHOR("Jay Fenlason <fenlason@redhat.com>");
-MODULE_DESCRIPTION("IPv4 over IEEE1394 as per RFC 2734");
+MODULE_DESCRIPTION("IP over IEEE1394 as per RFC 2734/3146");
MODULE_LICENSE("GPL");
MODULE_DEVICE_TABLE(ieee1394, fwnet_id_table);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index bd9f936..060a08a 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -70,6 +70,7 @@
#include <net/snmp.h>
#include <net/af_ieee802154.h>
+#include <net/firewire.h>
#include <net/ipv6.h>
#include <net/protocol.h>
#include <net/ndisc.h>
@@ -1660,6 +1661,20 @@ static int addrconf_ifid_eui64(u8 *eui, struct net_device *dev)
return 0;
}
+static int addrconf_ifid_ieee1394(u8 *eui, struct net_device *dev)
+{
+ union fwnet_hwaddr *ha;
+
+ if (dev->addr_len != FWNET_ALEN)
+ return -1;
+
+ ha = (union fwnet_hwaddr *)dev->dev_addr;
+
+ memcpy(eui, &ha->uc.uniq_id, sizeof(ha->uc.uniq_id));
+ eui[0] ^= 2;
+ return 0;
+}
+
static int addrconf_ifid_arcnet(u8 *eui, struct net_device *dev)
{
/* XXX: inherit EUI-64 from other interface -- yoshfuji */
@@ -1724,6 +1739,8 @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
return addrconf_ifid_gre(eui, dev);
case ARPHRD_IEEE802154:
return addrconf_ifid_eui64(eui, dev);
+ case ARPHRD_IEEE1394:
+ return addrconf_ifid_ieee1394(eui, dev);
}
return -1;
}
@@ -2565,7 +2582,8 @@ static void addrconf_dev_config(struct net_device *dev)
(dev->type != ARPHRD_FDDI) &&
(dev->type != ARPHRD_ARCNET) &&
(dev->type != ARPHRD_INFINIBAND) &&
- (dev->type != ARPHRD_IEEE802154)) {
+ (dev->type != ARPHRD_IEEE802154) &&
+ (dev->type != ARPHRD_IEEE1394)) {
/* Alas, we support only Ethernet autoconfiguration. */
return;
}
--
1.7.9.5
^ permalink raw reply related
* [RFC net-next (TAKE 3) 5/6] firewire net, ipv4 arp: Extend hardware address and remove driver-level packet inspection.
From: YOSHIFUJI Hideaki @ 2013-02-10 17:47 UTC (permalink / raw)
To: netdev, linux1394-devel; +Cc: stephan.gatzka, davem, stefanr, yoshfuji
Inspection of upper layer protocol is considered harmful, especially
if it is about ARP or other stateful upper layer protocol; driver
cannot (and should not) have full state of them.
IPv4 over Firewire module used to inspect ARP (both in sending path
and in receiving path), and record peer's GUID, max packet size, max
speed and fifo address. This patch removes such inspection by extending
our "hardware address" definition to include other information as well:
max packet size, max speed and fifo. By doing this, The neighbour
module in networking subsystem can cache them.
Note: As we have started ignoring sspd and max_rec in NDP, those
information will not be used in the driver when sending.
When a packet is being sent, the IP layer fills our pseudo header with
the extended "hardware address", including GUID and fifo. The driver
can look-up node-id (the real but rather volatile low-level address)
by GUID, and then the module can send the packet to the wire using
parameters provided in the extendedn hardware address.
This approach is realistic because IP over IEEE1394 (RFC2734) and IPv6
over IEEE1394 (RFC3146) share same "hardware address" format
in their address resolution protocols.
Here, extended "hardware address" is defined as follows:
union fwnet_hwaddr {
u8 u[16];
struct {
__be64 uniq_id; /* EUI-64 */
u8 max_rec; /* max packet size */
u8 sspd; /* max speed */
__be16 fifo_hi; /* hi 16bits of FIFO addr */
__be32 fifo_lo; /* lo 32bits of FIFO addr */
} __packed uc;
};
Note that Hardware address is declared as union, so that we can map full
IP address into this, when implementing MCAP (Multicast Cannel Allocation
Protocol) for IPv6, but IP and ARP subsystem do not need to know this
format in detail.
One difference between original ARP (RFC826) and 1394 ARP (RFC2734)
is that 1394 ARP Request/Reply do not contain the target hardware address
field (aka ar$tha). This difference is handled in the ARP subsystem.
CC: Stephan Gatzka <stephan.gatzka@gmail.com>
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
drivers/firewire/net.c | 153 ++++++++----------------------------------------
include/linux/if_arp.h | 12 +++-
include/net/firewire.h | 25 ++++++++
net/ipv4/arp.c | 18 ++++--
4 files changed, 74 insertions(+), 134 deletions(-)
create mode 100644 include/net/firewire.h
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 851aef0..177a092 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -28,6 +28,7 @@
#include <asm/unaligned.h>
#include <net/arp.h>
+#include <net/firewire.h>
/* rx limits */
#define FWNET_MAX_FRAGMENTS 30 /* arbitrary, > TX queue depth */
@@ -57,33 +58,6 @@
#define RFC2374_HDR_LASTFRAG 2 /* last fragment */
#define RFC2374_HDR_INTFRAG 3 /* interior fragment */
-#define RFC2734_HW_ADDR_LEN 16
-
-struct rfc2734_arp {
- __be16 hw_type; /* 0x0018 */
- __be16 proto_type; /* 0x0806 */
- u8 hw_addr_len; /* 16 */
- u8 ip_addr_len; /* 4 */
- __be16 opcode; /* ARP Opcode */
- /* Above is exactly the same format as struct arphdr */
-
- __be64 s_uniq_id; /* Sender's 64bit EUI */
- u8 max_rec; /* Sender's max packet size */
- u8 sspd; /* Sender's max speed */
- __be16 fifo_hi; /* hi 16bits of sender's FIFO addr */
- __be32 fifo_lo; /* lo 32bits of sender's FIFO addr */
- __be32 sip; /* Sender's IP Address */
- __be32 tip; /* IP Address of requested hw addr */
-} __packed;
-
-/* This header format is specific to this driver implementation. */
-#define FWNET_ALEN 8
-#define FWNET_HLEN 10
-struct fwnet_header {
- u8 h_dest[FWNET_ALEN]; /* destination address */
- __be16 h_proto; /* packet type ID field */
-} __packed;
-
static bool fwnet_hwaddr_is_multicast(u8 *ha)
{
return !!(*ha & 1);
@@ -196,8 +170,6 @@ struct fwnet_peer {
struct list_head peer_link;
struct fwnet_device *dev;
u64 guid;
- u64 fifo;
- __be32 ip;
/* guarded by dev->lock */
struct list_head pd_list; /* received partial datagrams */
@@ -227,6 +199,15 @@ struct fwnet_packet_task {
};
/*
+ * Get fifo address embedded in hwaddr
+ */
+static __u64 fwnet_hwaddr_fifo(union fwnet_hwaddr *ha)
+{
+ return (u64)get_unaligned_be16(&ha->uc.fifo_hi) << 32
+ | get_unaligned_be32(&ha->uc.fifo_lo);
+}
+
+/*
* saddr == NULL means use device source address.
* daddr == NULL means leave destination address (eg unresolved arp).
*/
@@ -518,7 +499,6 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
bool is_broadcast, u16 ether_type)
{
struct fwnet_device *dev;
- static const __be64 broadcast_hw = cpu_to_be64(~0ULL);
int status;
__be64 guid;
@@ -537,76 +517,11 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
/*
* Parse the encapsulation header. This actually does the job of
- * converting to an ethernet frame header, as well as arp
- * conversion if needed. ARP conversion is easier in this
- * direction, since we are using ethernet as our backend.
- */
- /*
- * If this is an ARP packet, convert it. First, we want to make
- * use of some of the fields, since they tell us a little bit
- * about the sending machine.
+ * converting to an ethernet-like pseudo frame header.
*/
- if (ether_type == ETH_P_ARP) {
- struct rfc2734_arp *arp1394;
- struct arphdr *arp;
- unsigned char *arp_ptr;
- u64 fifo_addr;
- u64 peer_guid;
- struct fwnet_peer *peer;
- unsigned long flags;
-
- arp1394 = (struct rfc2734_arp *)skb->data;
- arp = (struct arphdr *)skb->data;
- arp_ptr = (unsigned char *)(arp + 1);
- peer_guid = get_unaligned_be64(&arp1394->s_uniq_id);
- fifo_addr = (u64)get_unaligned_be16(&arp1394->fifo_hi) << 32
- | get_unaligned_be32(&arp1394->fifo_lo);
-
- spin_lock_irqsave(&dev->lock, flags);
- peer = fwnet_peer_find_by_guid(dev, peer_guid);
- if (peer) {
- peer->fifo = fifo_addr;
- peer->ip = arp1394->sip;
- }
- spin_unlock_irqrestore(&dev->lock, flags);
-
- if (!peer) {
- dev_notice(&net->dev,
- "no peer for ARP packet from %016llx\n",
- (unsigned long long)peer_guid);
- goto no_peer;
- }
-
- /*
- * Now that we're done with the 1394 specific stuff, we'll
- * need to alter some of the data. Believe it or not, all
- * that needs to be done is sender_IP_address needs to be
- * moved, the destination hardware address get stuffed
- * in and the hardware address length set to 8.
- *
- * IMPORTANT: The code below overwrites 1394 specific data
- * needed above so keep the munging of the data for the
- * higher level IP stack last.
- */
-
- arp->ar_hln = 8;
- /* skip over sender unique id */
- arp_ptr += arp->ar_hln;
- /* move sender IP addr */
- put_unaligned(arp1394->sip, (u32 *)arp_ptr);
- /* skip over sender IP addr */
- arp_ptr += arp->ar_pln;
-
- if (arp->ar_op == htons(ARPOP_REQUEST))
- memset(arp_ptr, 0, sizeof(u64));
- else
- memcpy(arp_ptr, net->dev_addr, sizeof(u64));
- }
-
- /* Now add the ethernet header. */
guid = cpu_to_be64(dev->card->guid);
if (dev_hard_header(skb, net, ether_type,
- is_broadcast ? &broadcast_hw : &guid,
+ is_broadcast ? net->broadcast : net->dev_addr,
NULL, skb->len) >= 0) {
struct fwnet_header *eth;
u16 *rawp;
@@ -649,7 +564,6 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
return 0;
- no_peer:
err:
net->stats.rx_errors++;
net->stats.rx_dropped++;
@@ -1307,11 +1221,12 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net)
ptask->dest_node = IEEE1394_ALL_NODES;
ptask->speed = SCODE_100;
} else {
- __be64 guid = get_unaligned((__be64 *)hdr_buf.h_dest);
+ union fwnet_hwaddr *ha = (union fwnet_hwaddr *)hdr_buf.h_dest;
+ __be64 guid = get_unaligned(&ha->uc.uniq_id);
u8 generation;
peer = fwnet_peer_find_by_guid(dev, be64_to_cpu(guid));
- if (!peer || peer->fifo == FWNET_NO_FIFO_ADDR)
+ if (!peer)
goto fail;
generation = peer->generation;
@@ -1319,32 +1234,12 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net)
max_payload = peer->max_payload;
datagram_label_ptr = &peer->datagram_label;
- ptask->fifo_addr = peer->fifo;
+ ptask->fifo_addr = fwnet_hwaddr_fifo(ha);
ptask->generation = generation;
ptask->dest_node = dest_node;
ptask->speed = peer->speed;
}
- /* If this is an ARP packet, convert it */
- if (proto == htons(ETH_P_ARP)) {
- struct arphdr *arp = (struct arphdr *)skb->data;
- unsigned char *arp_ptr = (unsigned char *)(arp + 1);
- struct rfc2734_arp *arp1394 = (struct rfc2734_arp *)skb->data;
- __be32 ipaddr;
-
- ipaddr = get_unaligned((__be32 *)(arp_ptr + FWNET_ALEN));
-
- arp1394->hw_addr_len = RFC2734_HW_ADDR_LEN;
- arp1394->max_rec = dev->card->max_receive;
- arp1394->sspd = dev->card->link_speed;
-
- put_unaligned_be16(dev->local_fifo >> 32,
- &arp1394->fifo_hi);
- put_unaligned_be32(dev->local_fifo & 0xffffffff,
- &arp1394->fifo_lo);
- put_unaligned(ipaddr, &arp1394->sip);
- }
-
ptask->hdr.w0 = 0;
ptask->hdr.w1 = 0;
ptask->skb = skb;
@@ -1459,8 +1354,6 @@ static int fwnet_add_peer(struct fwnet_device *dev,
peer->dev = dev;
peer->guid = (u64)device->config_rom[3] << 32 | device->config_rom[4];
- peer->fifo = FWNET_NO_FIFO_ADDR;
- peer->ip = 0;
INIT_LIST_HEAD(&peer->pd_list);
peer->pdg_size = 0;
peer->datagram_label = 0;
@@ -1490,6 +1383,7 @@ static int fwnet_probe(struct device *_dev)
struct fwnet_device *dev;
unsigned max_mtu;
int ret;
+ union fwnet_hwaddr *ha;
mutex_lock(&fwnet_device_mutex);
@@ -1538,8 +1432,15 @@ static int fwnet_probe(struct device *_dev)
net->mtu = min(1500U, max_mtu);
/* Set our hardware address while we're at it */
- put_unaligned_be64(card->guid, net->dev_addr);
- put_unaligned_be64(~0ULL, net->broadcast);
+ ha = (union fwnet_hwaddr *)net->dev_addr;
+ put_unaligned_be64(card->guid, &ha->uc.uniq_id);
+ ha->uc.max_rec = dev->card->max_receive;
+ ha->uc.sspd = dev->card->link_speed;
+ put_unaligned_be16(dev->local_fifo >> 32, &ha->uc.fifo_hi);
+ put_unaligned_be32(dev->local_fifo & 0xffffffff, &ha->uc.fifo_lo);
+
+ memset(net->broadcast, -1, net->addr_len);
+
ret = register_netdev(net);
if (ret)
goto out;
@@ -1591,8 +1492,6 @@ static int fwnet_remove(struct device *_dev)
mutex_lock(&fwnet_device_mutex);
net = dev->netdev;
- if (net && peer->ip)
- arp_invalidate(net, peer->ip);
fwnet_remove_peer(peer, dev);
diff --git a/include/linux/if_arp.h b/include/linux/if_arp.h
index 89b4614..f563907 100644
--- a/include/linux/if_arp.h
+++ b/include/linux/if_arp.h
@@ -33,7 +33,15 @@ static inline struct arphdr *arp_hdr(const struct sk_buff *skb)
static inline int arp_hdr_len(struct net_device *dev)
{
- /* ARP header, plus 2 device addresses, plus 2 IP addresses. */
- return sizeof(struct arphdr) + (dev->addr_len + sizeof(u32)) * 2;
+ switch (dev->type) {
+#if IS_ENABLED(CONFIG_FIREWIRE_NET)
+ case ARPHRD_IEEE1394:
+ /* ARP header, device address and 2 IP addresses */
+ return sizeof(struct arphdr) + dev->addr_len + sizeof(u32) * 2;
+#endif
+ default:
+ /* ARP header, plus 2 device addresses, plus 2 IP addresses. */
+ return sizeof(struct arphdr) + (dev->addr_len + sizeof(u32)) * 2;
+ }
}
#endif /* _LINUX_IF_ARP_H */
diff --git a/include/net/firewire.h b/include/net/firewire.h
new file mode 100644
index 0000000..31bcbfe
--- /dev/null
+++ b/include/net/firewire.h
@@ -0,0 +1,25 @@
+#ifndef _NET_FIREWIRE_H
+#define _NET_FIREWIRE_H
+
+/* Pseudo L2 address */
+#define FWNET_ALEN 16
+union fwnet_hwaddr {
+ u8 u[FWNET_ALEN];
+ /* "Hardware address" defined in RFC2734/RF3146 */
+ struct {
+ __be64 uniq_id; /* EUI-64 */
+ u8 max_rec; /* max packet size */
+ u8 sspd; /* max speed */
+ __be16 fifo_hi; /* hi 16bits of FIFO addr */
+ __be32 fifo_lo; /* lo 32bits of FIFO addr */
+ } __packed uc;
+};
+
+/* Pseudo L2 Header */
+#define FWNET_HLEN 18
+struct fwnet_header {
+ u8 h_dest[FWNET_ALEN]; /* destination address */
+ __be16 h_proto; /* packet type ID field */
+} __packed;
+
+#endif
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 9547a273..2db3af3 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -654,11 +654,19 @@ struct sk_buff *arp_create(int type, int ptype, __be32 dest_ip,
arp_ptr += dev->addr_len;
memcpy(arp_ptr, &src_ip, 4);
arp_ptr += 4;
- if (target_hw != NULL)
- memcpy(arp_ptr, target_hw, dev->addr_len);
- else
- memset(arp_ptr, 0, dev->addr_len);
- arp_ptr += dev->addr_len;
+
+ switch (dev->type) {
+#if IS_ENABLED(CONFIG_FIREWIRE_NET)
+ case ARPHRD_IEEE1394:
+ break;
+#endif
+ default:
+ if (target_hw != NULL)
+ memcpy(arp_ptr, target_hw, dev->addr_len);
+ else
+ memset(arp_ptr, 0, dev->addr_len);
+ arp_ptr += dev->addr_len;
+ }
memcpy(arp_ptr, &dest_ip, 4);
return skb;
--
1.7.9.5
^ permalink raw reply related
* [RFC net-next (TAKE 3) 4/6] firewire net: Allocate address handler before registering net_device.
From: YOSHIFUJI Hideaki @ 2013-02-10 17:47 UTC (permalink / raw)
To: netdev, linux1394-devel; +Cc: stephan.gatzka, davem, stefanr, yoshfuji
Since we are going to embedding our FIFO address into our pseudo HW
address, let's allocate FIFO address before registering net_device.
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
drivers/firewire/net.c | 37 +++++++++++++++++--------------------
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 2c263de..851aef0 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -1124,19 +1124,6 @@ static int fwnet_broadcast_start(struct fwnet_device *dev)
unsigned long offset;
unsigned u;
- if (dev->local_fifo == FWNET_NO_FIFO_ADDR) {
- dev->handler.length = 4096;
- dev->handler.address_callback = fwnet_receive_packet;
- dev->handler.callback_data = dev;
-
- retval = fw_core_add_address_handler(&dev->handler,
- &fw_high_memory_region);
- if (retval < 0)
- goto failed_initial;
-
- dev->local_fifo = dev->handler.offset;
- }
-
max_receive = 1U << (dev->card->max_receive + 1);
num_packets = (FWNET_ISO_PAGE_COUNT * PAGE_SIZE) / max_receive;
@@ -1217,9 +1204,6 @@ static int fwnet_broadcast_start(struct fwnet_device *dev)
fw_iso_context_destroy(context);
dev->broadcast_rcv_context = NULL;
failed_context_create:
- fw_core_remove_address_handler(&dev->handler);
- failed_initial:
- dev->local_fifo = FWNET_NO_FIFO_ADDR;
return retval;
}
@@ -1536,6 +1520,15 @@ static int fwnet_probe(struct device *_dev)
dev->card = card;
dev->netdev = net;
+ dev->handler.length = 4096;
+ dev->handler.address_callback = fwnet_receive_packet;
+ dev->handler.callback_data = dev;
+
+ ret = fw_core_add_address_handler(&dev->handler, &fw_high_memory_region);
+ if (ret < 0)
+ goto out;
+ dev->local_fifo = dev->handler.offset;
+
/*
* Use the RFC 2734 default 1500 octets or the maximum payload
* as initial MTU
@@ -1559,10 +1552,13 @@ static int fwnet_probe(struct device *_dev)
if (ret && allocated_netdev) {
unregister_netdev(net);
list_del(&dev->dev_link);
- }
out:
- if (ret && allocated_netdev)
+ if (dev->local_fifo != FWNET_NO_FIFO_ADDR) {
+ fw_core_remove_address_handler(&dev->handler);
+ dev->local_fifo = FWNET_NO_FIFO_ADDR;
+ }
free_netdev(net);
+ }
mutex_unlock(&fwnet_device_mutex);
@@ -1603,8 +1599,9 @@ static int fwnet_remove(struct device *_dev)
if (list_empty(&dev->peer_list)) {
unregister_netdev(net);
- if (dev->local_fifo != FWNET_NO_FIFO_ADDR)
- fw_core_remove_address_handler(&dev->handler);
+ fw_core_remove_address_handler(&dev->handler);
+ dev->local_fifo = FWNET_NO_FIFO_ADDR;
+
if (dev->broadcast_rcv_context) {
fw_iso_context_stop(dev->broadcast_rcv_context);
fw_iso_buffer_destroy(&dev->broadcast_rcv_buffer,
--
1.7.9.5
^ permalink raw reply related
* [RFC net-next (TAKE 3) 3/6] firewire net: Send L2 multicast via GASP.
From: YOSHIFUJI Hideaki @ 2013-02-10 17:47 UTC (permalink / raw)
To: netdev, linux1394-devel; +Cc: stephan.gatzka, davem, stefanr, yoshfuji
Send L2 multicast packet via GASP, not by seeing upper-layer
protocol type/address.
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
drivers/firewire/net.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 98d3ec7..2c263de 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -84,6 +84,11 @@ struct fwnet_header {
__be16 h_proto; /* packet type ID field */
} __packed;
+static bool fwnet_hwaddr_is_multicast(u8 *ha)
+{
+ return !!(*ha & 1);
+}
+
/* IPv4 and IPv6 encapsulation header */
struct rfc2734_header {
u32 w0;
@@ -610,7 +615,7 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
skb_reset_mac_header(skb);
skb_pull(skb, sizeof(*eth));
eth = (struct fwnet_header *)skb_mac_header(skb);
- if (*eth->h_dest & 1) {
+ if (fwnet_hwaddr_is_multicast(eth->h_dest)) {
if (memcmp(eth->h_dest, net->broadcast,
net->addr_len) == 0)
skb->pkt_type = PACKET_BROADCAST;
@@ -1309,10 +1314,7 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net)
* Set the transmission type for the packet. ARP packets and IP
* broadcast packets are sent via GASP.
*/
- if (memcmp(hdr_buf.h_dest, net->broadcast, FWNET_ALEN) == 0
- || proto == htons(ETH_P_ARP)
- || (proto == htons(ETH_P_IP)
- && IN_MULTICAST(ntohl(ip_hdr(skb)->daddr)))) {
+ if (fwnet_hwaddr_is_multicast(hdr_buf.h_dest)) {
max_payload = dev->broadcast_xmt_max_payload;
datagram_label_ptr = &dev->broadcast_xmt_datagramlabel;
--
1.7.9.5
^ permalink raw reply related
* [RFC net-next (TAKE 3) 2/6] firewire net: Accept IPv4 and ARP only.
From: YOSHIFUJI Hideaki @ 2013-02-10 17:46 UTC (permalink / raw)
To: netdev, linux1394-devel; +Cc: stephan.gatzka, davem, stefanr, yoshfuji
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
drivers/firewire/net.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 4382c20..98d3ec7 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -517,6 +517,14 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
int status;
__be64 guid;
+ switch (ether_type) {
+ case ETH_P_ARP:
+ case ETH_P_IP:
+ break;
+ default:
+ goto err;
+ }
+
dev = netdev_priv(net);
/* Write metadata, and then pass to the receive level */
skb->dev = net;
@@ -637,6 +645,7 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
return 0;
no_peer:
+ err:
net->stats.rx_errors++;
net->stats.rx_dropped++;
@@ -1283,9 +1292,17 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net)
* We might need to rebuild the header on tx failure.
*/
memcpy(&hdr_buf, skb->data, sizeof(hdr_buf));
- skb_pull(skb, sizeof(hdr_buf));
-
proto = hdr_buf.h_proto;
+
+ switch (proto) {
+ case htons(ETH_P_ARP):
+ case htons(ETH_P_IP):
+ break;
+ default:
+ goto fail;
+ }
+
+ skb_pull(skb, sizeof(hdr_buf));
dg_size = skb->len;
/*
--
1.7.9.5
^ permalink raw reply related
* [RFC net-next (TAKE 3) 1/6] firewire net: Ignore spd and max_payload advertised by ARP.
From: YOSHIFUJI Hideaki @ 2013-02-10 17:46 UTC (permalink / raw)
To: netdev, linux1394-devel; +Cc: stephan.gatzka, davem, stefanr, yoshfuji
Stefan Richter <stefanr@s5r6.in-berlin.de> says:
| As far as I can tell, it would be best to ignore max_rec and sspd from ARP
| and NDP but keep using the respective information from firewire-core
| instead (handed over by fwnet_probe()).
|
| Why? As I noted earlier, RFC 2734:1999 and RFC 3146:2001 were apparently
| written with a too simplistic notion of IEEE 1394 bus topology, resulting
| in max_rec and sspd in ARP-1394 and NDP-1394 to be useless, IMO.
|
| Consider a bus like this:
|
| A ---- B ==== C
|
| A, B, C are all IP-over-1394 capable nodes. ---- is an S400 cable hop,
| and ==== is an S800 cable hop.
|
| In case of unicasts or multicasts in which node A is involved as
| transmitter or receiver, as well as in case of broadcasts, the speeds
| S100, S200, S400 work and speed S400 is optimal.
|
| In case of anything else, IOW in case of unicasts or multicasts in which
| only nodes B and C are involved, the speeds S100, S200, S400, S800 work
| and speed S800 is optimal.
|
| Clearly, node A should indicate sspd = S400 in its ARP or NDP packets.
| But which sspd should nodes B and C set there? Maybe they set S400, which
| would work but would waste half of the available bandwidth in the second
| case. Or maybe they set S800, which is OK in the second case but would
| prohibit any communication with node A if blindly taken for correct.
|
| On the other hand, firewire-core *always* gives us the correct and optimum
| peer-to-peer speed and asynchronous packet payload, no matter how simple
| or complex the bus topology is and no matter in which temporal order nodes
| join the bus and are discovered.
CC: Stefan Richter <stefanr@s5r6.in-berlin.de>
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
drivers/firewire/net.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 2b27bff..4382c20 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -539,8 +539,6 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
unsigned char *arp_ptr;
u64 fifo_addr;
u64 peer_guid;
- unsigned sspd;
- u16 max_payload;
struct fwnet_peer *peer;
unsigned long flags;
@@ -551,24 +549,10 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
fifo_addr = (u64)get_unaligned_be16(&arp1394->fifo_hi) << 32
| get_unaligned_be32(&arp1394->fifo_lo);
- sspd = arp1394->sspd;
- /* Sanity check. OS X 10.3 PPC reportedly sends 131. */
- if (sspd > SCODE_3200) {
- dev_notice(&net->dev, "sspd %x out of range\n", sspd);
- sspd = SCODE_3200;
- }
- max_payload = fwnet_max_payload(arp1394->max_rec, sspd);
-
spin_lock_irqsave(&dev->lock, flags);
peer = fwnet_peer_find_by_guid(dev, peer_guid);
if (peer) {
peer->fifo = fifo_addr;
-
- if (peer->speed > sspd)
- peer->speed = sspd;
- if (peer->max_payload > max_payload)
- peer->max_payload = max_payload;
-
peer->ip = arp1394->sip;
}
spin_unlock_irqrestore(&dev->lock, flags);
--
1.7.9.5
^ permalink raw reply related
* [RFC net-next (TAKE 3) 0/6] IPv6 over Firewire
From: YOSHIFUJI Hideaki @ 2013-02-10 17:46 UTC (permalink / raw)
To: netdev, linux1394-devel; +Cc: stephan.gatzka, davem, stefanr, yoshfuji
This is take3 of IPv6 over Firewire.
- Ignore spd and max_payload in ARP/NDP.
- Separate patch for address handler related change
with some fixes.
- Fix minor typoes.
YOSHIFUJI Hideaki (6):
firewire net: Ignore spd and max_payload advertised by ARP.
firewire net: Accept IPv4 and ARP only.
firewire net: Send L2 multicast via GASP.
firewire net: Allocate address handler before registering net_device.
firewire net, ipv4 arp: Extend hardware address and remove
driver-level packet inspection.
ipv6: IPv6 over Firewire (RFC3146) support.
drivers/firewire/Kconfig | 6 +-
drivers/firewire/net.c | 322 +++++++++++++++++++++-------------------------
include/linux/if_arp.h | 12 +-
include/net/firewire.h | 25 ++++
net/ipv4/arp.c | 18 ++-
net/ipv6/addrconf.c | 20 ++-
6 files changed, 214 insertions(+), 189 deletions(-)
create mode 100644 include/net/firewire.h
--
1.7.9.5
^ permalink raw reply
* Re: [net-next (TAKE 2) 3/4] firewire net, ipv4 arp: Extend hardware address and remove driver-level packet inspection.
From: YOSHIFUJI Hideaki @ 2013-02-10 16:32 UTC (permalink / raw)
To: Stefan Richter; +Cc: netdev, linux1394-devel, davem, YOSHIFUJI Hideaki
In-Reply-To: <20130210151056.3e6e9f53@stein>
(2013年02月10日 23:10), Stefan Richter wrote:
> On Feb 10 YOSHIFUJI Hideaki wrote:
>> @@ -1140,18 +1051,6 @@ static int fwnet_broadcast_start(struct fwnet_device *dev)
>> unsigned long offset;
>> unsigned u;
>>
>> - if (dev->local_fifo == FWNET_NO_FIFO_ADDR) {
>> - dev->handler.length = 4096;
>> - dev->handler.address_callback = fwnet_receive_packet;
>> - dev->handler.callback_data = dev;
>> -
>> - retval = fw_core_add_address_handler(&dev->handler,
>> - &fw_high_memory_region);
>> - if (retval < 0)
>> - goto failed_initial;
>> -
>> - dev->local_fifo = dev->handler.offset;
>> - }
>>
>> max_receive = 1U << (dev->card->max_receive + 1);
>> num_packets = (FWNET_ISO_PAGE_COUNT * PAGE_SIZE) / max_receive;
>> @@ -1234,8 +1133,6 @@ static int fwnet_broadcast_start(struct fwnet_device *dev)
>> dev->broadcast_rcv_context = NULL;
>> failed_context_create:
>> fw_core_remove_address_handler(&dev->handler);
>> - failed_initial:
>> - dev->local_fifo = FWNET_NO_FIFO_ADDR;
>>
>> return retval;
>> }
>
> [...]
>
>> @@ -1546,12 +1421,21 @@ static int fwnet_probe(struct device *_dev)
>> dev->broadcast_rcv_context = NULL;
>> dev->broadcast_xmt_max_payload = 0;
>> dev->broadcast_xmt_datagramlabel = 0;
>> - dev->local_fifo = FWNET_NO_FIFO_ADDR;
>> dev->queued_datagrams = 0;
>> INIT_LIST_HEAD(&dev->peer_list);
>> dev->card = card;
>> dev->netdev = net;
>>
>> + dev->handler.length = 4096;
>> + dev->handler.address_callback = fwnet_receive_packet;
>> + dev->handler.callback_data = dev;
>> +
>> + ret = fw_core_add_address_handler(&dev->handler, &fw_high_memory_region);
>> + if (ret < 0)
>> + goto out;
>> +
>> + dev->local_fifo = dev->handler.offset;
>> +
>> /*
>> * Use the RFC 2734 default 1500 octets or the maximum payload
>> * as initial MTU
>
> If at all possible, please put changes of this kind into a separate
> patch.
>
> I get a distinct feeling that the error handling paths in
> fwnet_broadcast_start() and in fwnet_probe() respectively became buggy now.
> This would be easier to verify if shifting this code was done in a separate
> change.
Will do.
--yoshfuji
^ permalink raw reply
* [PATCH 2/2] net: calexdaxgmac: fixup endian issues after __raw IO function change
From: Ben Dooks @ 2013-02-10 15:38 UTC (permalink / raw)
To: netdev; +Cc: linux-arm-kernel, Ben Dooks, David S. Miller, Rob Herring
In-Reply-To: <1360510721-17860-1-git-send-email-ben.dooks@codethink.co.uk>
When changing to __raw acccessors in 0ec6d343f7bcf9e0944aa9ff65287b987ec00c0f
("net: calxedaxgmac: use raw i/o accessors in rx and tx paths"), the driver
is now broken on big endian systems as the readl/writel have an implict
endian swap in them.
Change all the places where the __raw calls are used to correctly convert
the constants in big endian format to the little endian data that the
peripheral expects to see.
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
drivers/net/ethernet/calxeda/xgmac.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/calxeda/xgmac.c b/drivers/net/ethernet/calxeda/xgmac.c
index f91d9b2..96fd538 100644
--- a/drivers/net/ethernet/calxeda/xgmac.c
+++ b/drivers/net/ethernet/calxeda/xgmac.c
@@ -1202,7 +1202,8 @@ static int xgmac_poll(struct napi_struct *napi, int budget)
if (work_done < budget) {
napi_complete(napi);
- __raw_writel(DMA_INTR_DEFAULT_MASK, priv->base + XGMAC_DMA_INTR_ENA);
+ __raw_writel(le32_to_cpu((__force __le32)DMA_INTR_DEFAULT_MASK),
+ priv->base + XGMAC_DMA_INTR_ENA);
}
return work_done;
}
@@ -1348,7 +1349,7 @@ static irqreturn_t xgmac_pmt_interrupt(int irq, void *dev_id)
void __iomem *ioaddr = priv->base;
intr_status = __raw_readl(ioaddr + XGMAC_INT_STAT);
- if (intr_status & XGMAC_INT_STAT_PMT) {
+ if (intr_status & le32_to_cpu((__force __le32)XGMAC_INT_STAT_PMT)) {
netdev_dbg(priv->dev, "received Magic frame\n");
/* clear the PMT bits 5 and 6 by reading the PMT */
readl(ioaddr + XGMAC_PMT);
@@ -1369,6 +1370,8 @@ static irqreturn_t xgmac_interrupt(int irq, void *dev_id)
intr_status &= __raw_readl(priv->base + XGMAC_DMA_INTR_ENA);
__raw_writel(intr_status, priv->base + XGMAC_DMA_STATUS);
+ intr_status = (__force u32)cpu_to_le32(intr_status);
+
/* It displays the DMA process states (CSR5 register) */
/* ABNORMAL interrupts */
if (unlikely(intr_status & DMA_STATUS_AIS)) {
@@ -1403,7 +1406,8 @@ static irqreturn_t xgmac_interrupt(int irq, void *dev_id)
/* TX/RX NORMAL interrupts */
if (intr_status & (DMA_STATUS_RI | DMA_STATUS_TU | DMA_STATUS_TI)) {
- __raw_writel(DMA_INTR_ABNORMAL, priv->base + XGMAC_DMA_INTR_ENA);
+ __raw_writel(le32_to_cpu((__force __le32)DMA_INTR_ABNORMAL),
+ priv->base + XGMAC_DMA_INTR_ENA);
napi_schedule(&priv->napi);
}
--
1.7.10.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox