First of all thanks for the feedback. I know the fgar_swap_*bits
function is a bit ugly. The problem with your suggestion is that it is
necessary to swap a1<>a2 and b1<>b2 AT THE SAME TIME. Otherwise
information the second swap is needing is swapped away by the first one.
If you would do this as a macro I guess it will be blown up and be as
bad as it is at the moment:
#define swap_bits(a, b, c, d, bits) \
do { \
typeof(a) _bits = bits; \
typeof(a) _a_bits = (a) & _bits; \
typeof(a) _b_bits = (b) & _bits; \
typeof(a) _c_bits = (c) & _bits; \
typeof(a) _d_bits = (d) & _bits; \
\
(a) &= ~_bits; \
(b) &= ~_bits; \
(c) &= ~_bits; \
(d) &= ~_bits; \
\
(a) |= _b_bits; \
(b) |= _a_bits; \
(c) |= _d_bits; \
(d) |= _c_bits; \
} while (0)
quoted
+/* Swaps the 0xFF80 masked bits of a1<>a2 and b1<>b2 */
+static void gfar_swap_ff80_bits(struct gfar_filer_entry *a1,
+ struct gfar_filer_entry *a2, struct gfar_filer_entry *b1,
+ struct gfar_filer_entry *b2)
+{
+ u32 temp[4];
+ temp[0] = a1->ctrl & 0xFF80;
+ temp[1] = a2->ctrl & 0xFF80;
+ temp[2] = b1->ctrl & 0xFF80;
+ temp[3] = b2->ctrl & 0xFF80;
+
+ a1->ctrl &= ~0xFF80;
+ a2->ctrl &= ~0xFF80;
+ b1->ctrl &= ~0xFF80;
+ b2->ctrl &= ~0xFF80;
+
+ a1->ctrl |= temp[1];
+ a2->ctrl |= temp[0];
+ b1->ctrl |= temp[3];
+ b2->ctrl |= temp[2];
+}
maybe add a macro similar to swap:
#define swap_bits(a, b, bits) \
do { \
typeof(a) _bits = bits; \
typeof(a) _a_bits = (a) & _bits; \
typeof(a) _b_bits = (b) & _bits; \
\
(a) &= ~_bits; \
(b) &= ~_bits; \
\
(a) |= _b_bits; \
(b) |= _a_bits; \
} while (0)
and use this macro directly in gfar_sort_mask_table?
swap_bits(temp_table->fe[new_first],
temp_table->fe[old_first], 0xff80);
swap_bits(temp_table->fe[new_last],
temp_table->fe[old_last], 0xff80);
And maybe 0xff80 should be a #define?
Signed-off-by: Sebastian Poehn <redacted>
---
drivers/net/gianfar.h | 1 +
drivers/net/gianfar_ethtool.c | 29 ++++++++++++++++-------------
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/drivers/net/gianfar.h b/drivers/net/gianfar.h
index 76f14d0..27499c6 100644
--- a/drivers/net/gianfar.h
+++ b/drivers/net/gianfar.h
@@ -409,6 +409,7 @@ extern const char gfar_driver_version[];
#define RQFCR_HASHTBL_2 0x00060000
#define RQFCR_HASHTBL_3 0x00080000
#define RQFCR_HASH 0x00010000
+#define RQFCR_QUEUE 0x0000FC00
#define RQFCR_CLE 0x00000200
#define RQFCR_RJE 0x00000100
#define RQFCR_AND 0x00000080
diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
index 2ecdc9a..ee93bb8 100644
--- a/drivers/net/gianfar_ethtool.c
+++ b/drivers/net/gianfar_ethtool.c
@@ -1262,21 +1262,21 @@ static void gfar_cluster_filer(struct filer_table *tab)
}
}
-/* Swaps the 0xFF80 masked bits of a1<>a2 and b1<>b2 */
-static void gfar_swap_ff80_bits(struct gfar_filer_entry *a1,
+/* Swaps the masked bits of a1<>a2 and b1<>b2 */
+static void gfar_swap_bits(struct gfar_filer_entry *a1,
struct gfar_filer_entry *a2, struct gfar_filer_entry *b1,
- struct gfar_filer_entry *b2)
+ struct gfar_filer_entry *b2, u32 mask)
{
u32 temp[4];
- temp[0] = a1->ctrl & 0xFF80;
- temp[1] = a2->ctrl & 0xFF80;
- temp[2] = b1->ctrl & 0xFF80;
- temp[3] = b2->ctrl & 0xFF80;
+ temp[0] = a1->ctrl & mask;
+ temp[1] = a2->ctrl & mask;
+ temp[2] = b1->ctrl & mask;
+ temp[3] = b2->ctrl & mask;
- a1->ctrl &= ~0xFF80;
- a2->ctrl &= ~0xFF80;
- b1->ctrl &= ~0xFF80;
- b2->ctrl &= ~0xFF80;
+ a1->ctrl &= ~mask;
+ a2->ctrl &= ~mask;
+ b1->ctrl &= ~mask;
+ b2->ctrl &= ~mask;
a1->ctrl |= temp[1];
a2->ctrl |= temp[0];@@ -1356,10 +1356,13 @@ static void gfar_sort_mask_table(struct gfar_mask_entry *mask_table,
new_first = mask_table[start].start + 1;
new_last = mask_table[i - 1].end;
- gfar_swap_ff80_bits(&temp_table->fe[new_first],
+ gfar_swap_bits(&temp_table->fe[new_first],
&temp_table->fe[old_first],
&temp_table->fe[new_last],
- &temp_table->fe[old_last]);
+ &temp_table->fe[old_last],
+ RQFCR_QUEUE | RQFCR_CLE |
+ RQFCR_RJE | RQFCR_AND
+ );
start = i;
size = 0;
On Wed, 2011-06-22 at 11:29 +0200, Sebastian Pöhn wrote:
First of all thanks for the feedback. I know the fgar_swap_*bits
function is a bit ugly. The problem with your suggestion is that it is
necessary to swap a1<>a2 and b1<>b2 AT THE SAME TIME.
These swaps are done sequentially.
Otherwise
information the second swap is needing is swapped away by the first one.
How so?
quoted
swap_bits(temp_table->fe[new_first],
temp_table->fe[old_first], 0xff80);
swap_bits(temp_table->fe[new_last],
temp_table->fe[old_last], 0xff80);
new_first and old_first are swapped.
new_last and old_last are swapped.
quoted hunk
@@ -1356,10 +1356,13 @@ static void gfar_sort_mask_table(struct gfar_mask_entry *mask_table,
new_first = mask_table[start].start + 1;
new_last = mask_table[i - 1].end;
I'm a bit confused by this:
+ if (prev != mask_table[i].block) {
+ old_first = mask_table[start].start + 1;
+ old_last = mask_table[i - 1].end;
[no changes to start or i]
+ new_first = mask_table[start].start + 1;
+ new_last = mask_table[i - 1].end;
+
+ gfar_swap_ff80_bits(&temp_table->fe[new_first],
+ &temp_table->fe[old_first],
+ &temp_table->fe[new_last],
+ &temp_table->fe[old_last]);
Doesn't this just swap the same bits around twice?
Thanks for your feedback Joe!
I'm a bit confused by this:
/*
* What we already have:
* A temporary copy of the filer table which will later replace the current one.
* In the filer table we always have one mask entry followed by a value (of action).
* Further we have the mask table which is a list of all mask in the filer table.
* To every entry we have the start and the end of the validity and a block index
* (same block indexes means dependency). We want to sort every dependent block!
*/
static void gfar_sort_mask_table(struct gfar_mask_entry *mask_table,
struct filer_table *temp_table, u32 and_index)
{
/* Pointer to compare function (_asc or _desc) */
int (*gfar_comp)(const void *, const void *);
u32 i, size = 0, start = 0, prev = 1;
u32 old_first, old_last, new_first, new_last;
gfar_comp = &gfar_comp_desc;
for (i = 0; i < and_index; i++) {
/* As long, as the block is the same (dependency) we only increase
* the size */
if (prev != mask_table[i].block) {
/* The +1 is because start is a mask and the important
* FF80 information is located in the next value element */
old_first = mask_table[start].start + 1;
/* -1 because we are already at the start of the next dependent block */
old_last = mask_table[i - 1].end;
/* Here the mask table is sorted! So the entries in the current
* block change may change their location */
sort(mask_table + start, size,
sizeof(struct gfar_mask_entry),
gfar_comp, &gfar_swap);
/* Toggle order for every block. This makes the
* thing more efficient! */
if (gfar_comp == gfar_comp_desc)
gfar_comp = &gfar_comp_asc;
else
gfar_comp = &gfar_comp_desc;
/* The same as above but the elements are others because of the sorting */
new_first = mask_table[start].start + 1;
new_last = mask_table[i - 1].end;
gfar_swap_bits(&temp_table->fe[new_first],
&temp_table->fe[old_first],
&temp_table->fe[new_last],
&temp_table->fe[old_last],
RQFCR_QUEUE | RQFCR_CLE |
RQFCR_RJE | RQFCR_AND
);
start = i;
size = 0;
}
size++;
prev = mask_table[i].block;
}
}
Doesn't this just swap the same bits around twice?
Due to the masks are sorted numerically it is possible that a mask from any location changes its locations to ANY. Lets think of this example and the usage of the sequential swapping:
A has a header <- old_first new position after sorting: 2
B <- new_last new position after sorting: 4
C new position after sorting: 3
D has a trailer <- old_last and new_first new position after sorting: 1
First swap first:
A has a trailer
B
C
D has a header
Second swap:
A has a trailer
B has a header
C
D
And now after the sorted order is applied:
D
A has a trailer
C
B has a header
Thats not correct!
The concurrent swapper does:
A
B has a trailer
C
D has a header
And:
D has a header
A
C
B has a trailer
That's all right!
DISCLAIMER:
Privileged and/or Confidential information may be contained in this
message. If you are not the addressee of this message, you may not
copy, use or deliver this message to anyone. In such event, you
should destroy the message and kindly notify the sender by reply
e-mail. It is understood that opinions or conclusions that do not
relate to the official business of the company are neither given
nor endorsed by the company.
Thank You.