| Message ID | 49340DAE.5060603@hartkopp.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote: > can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter > > Due to a wrong safety check in af_can.c it was not possible to filter > for SFF frames with a specific CAN identifier without getting the > same selected CAN identifier from a received EFF frame also. > > This fix has a minimum (but user visible) impact on the CAN filter > API and therefore the CAN version is set to a new date. > > Indeed the 'old' API is still working as-is. But when now setting > CAN_(EFF|RTR)_FLAG in can_filter.can_mask you might get less traffic > than before - but still the stuff that you expected to get for your > defined filter ... > > Thanks to Kurt Van Dijck for pointing at this issue and for the review. > > Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net> > Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be> If Kurt tested this patch then he should be credited with a "Tested-by:" tag. I recall he did but I deleted the thread from my can mailbox. Sam -- 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
Sam Ravnborg wrote: > On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote: > >> >> Thanks to Kurt Van Dijck for pointing at this issue and for the review. >> >> Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net> >> Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be> >> > > If Kurt tested this patch then he should > be credited with a "Tested-by:" tag. > > I recall he did but I deleted the thread from my can mailbox. > > Sam > Hi Sam, i did some testing and posted the results of a first patch that went into the final direction. In https://lists.berlios.de/pipermail/socketcan-core/2008-December/002117.html Kurt gave a "Reviewed-by:" and in a later mail in https://lists.berlios.de/pipermail/socketcan-core/2008-December/002119.html he wrote "my Acked-by: still stands" for the final patch. So i choosed "Acked-by:". As Kurt went that deep into the code that he pointed me to the right line and presented a first idea for a patch, an 'Acked-by:' for confirming the final patch looked like an appropriate credit to me. Regards, Oliver -- 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
On Mon, Dec 01, 2008 at 07:42:31PM +0100, Oliver Hartkopp wrote: > Sam Ravnborg wrote: > >On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote: > > > >> > >>Thanks to Kurt Van Dijck for pointing at this issue and for the review. > >> > >>Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net> > >>Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be> > >> > > > >If Kurt tested this patch then he should > >be credited with a "Tested-by:" tag. > > > >I recall he did but I deleted the thread from my can mailbox. > > > > Sam > > > Hi Sam, > > i did some testing and posted the results of a first patch that went > into the final direction. > > In > > https://lists.berlios.de/pipermail/socketcan-core/2008-December/002117.html > > Kurt gave a "Reviewed-by:" and in a later mail in > > https://lists.berlios.de/pipermail/socketcan-core/2008-December/002119.html > > he wrote "my Acked-by: still stands" for the final patch. > > So i choosed "Acked-by:". > > As Kurt went that deep into the code that he pointed me to the right > line and presented a first idea for a patch, an 'Acked-by:' for > confirming the final patch looked like an appropriate credit to me. OK and thanks for the clarification Oliver. Sam -- 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
On Mon, Dec 01, 2008 at 07:42:31PM +0100, Oliver Hartkopp wrote: > Sam Ravnborg wrote: >> On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote: >> >>> >>> Thanks to Kurt Van Dijck for pointing at this issue and for the review. >>> >>> Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net> >>> Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be> >>> >> >> If Kurt tested this patch then he should >> be credited with a "Tested-by:" tag. >> >> I recall he did but I deleted the thread from my can mailbox. >> >> Sam >> > Hi Sam, > > i did some testing and posted the results of a first patch that went > into the final direction. > > In > > https://lists.berlios.de/pipermail/socketcan-core/2008-December/002117.html > > Kurt gave a "Reviewed-by:" and in a later mail in > > https://lists.berlios.de/pipermail/socketcan-core/2008-December/002119.html > > he wrote "my Acked-by: still stands" for the final patch. > > So i choosed "Acked-by:". > > As Kurt went that deep into the code that he pointed me to the right > line and presented a first idea for a patch, an 'Acked-by:' for > confirming the final patch looked like an appropriate credit to me. Well, I did not physically test Oliver's final patch, which looked totally different. By lack of time, did not take the effort of completely testing it again. The test scenario included the problematic scenario, which made me think Oliver understood the problem. For the final patch, a 'Acked-by:' looks appropriate to me too. Sam, I'm pleased to notice the credits are watched this close too. > > Regards, > Oliver > Kurt -- 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
From: Oliver Hartkopp <oliver@hartkopp.net> Date: Mon, 01 Dec 2008 17:15:42 +0100 > can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter > > Due to a wrong safety check in af_can.c it was not possible to filter > for SFF frames with a specific CAN identifier without getting the > same selected CAN identifier from a received EFF frame also. > > This fix has a minimum (but user visible) impact on the CAN filter > API and therefore the CAN version is set to a new date. > > Indeed the 'old' API is still working as-is. But when now setting > CAN_(EFF|RTR)_FLAG in can_filter.can_mask you might get less traffic > than before - but still the stuff that you expected to get for your > defined filter ... > > Thanks to Kurt Van Dijck for pointing at this issue and for the review. > > Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net> > Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be> Applied to net-2.6 But you are totally kidding yourself if you think that bumping that completely stupid date string has any meaning at all for userspace. You're either keeping to the API you're exporting to userspace or you're not. And using a date string which userland has to test is just a cop-out for not breaking userspace interfaces. -- 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
David Miller wrote: > From: Oliver Hartkopp <oliver@hartkopp.net> > Date: Mon, 01 Dec 2008 17:15:42 +0100 > > >> can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter >> >> > > Applied to net-2.6 > Thanks! > But you are totally kidding yourself if you think that bumping > that completely stupid date string has any meaning at all for > userspace. > > You're either keeping to the API you're exporting to userspace > or you're not. And using a date string which userland has to > test is just a cop-out for not breaking userspace interfaces. > This fix does fortunately not break the userspace interface. The version string is just for information and should not be used to determine changing userspace interfaces. Thinking about your concerns it may be the best approach to remove this version stuff entirely, before people begin to use it in any weird manner :-/ I'll make a RFC on the CAN ML if it can be removed. Thanks for the feedback, Oliver -- 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
diff --git a/include/linux/can/core.h b/include/linux/can/core.h index e9ca210..f50785a 100644 --- a/include/linux/can/core.h +++ b/include/linux/can/core.h @@ -19,7 +19,7 @@ #include <linux/skbuff.h> #include <linux/netdevice.h> -#define CAN_VERSION "20071116" +#define CAN_VERSION "20081130" /* increment this number each time you change some user-space interface */ #define CAN_ABI_VERSION "8" diff --git a/net/can/af_can.c b/net/can/af_can.c index 7d4d2b3..d8173e5 100644 --- a/net/can/af_can.c +++ b/net/can/af_can.c @@ -319,23 +319,52 @@ static struct dev_rcv_lists *find_dev_rcv_lists(struct net_device *dev) return n ? d : NULL; } +/** + * find_rcv_list - determine optimal filterlist inside device filter struct + * @can_id: pointer to CAN identifier of a given can_filter + * @mask: pointer to CAN mask of a given can_filter + * @d: pointer to the device filter struct + * + * Description: + * Returns the optimal filterlist to reduce the filter handling in the + * receive path. This function is called by service functions that need + * to register or unregister a can_filter in the filter lists. + * + * A filter matches in general, when + * + * <received_can_id> & mask == can_id & mask + * + * so every bit set in the mask (even CAN_EFF_FLAG, CAN_RTR_FLAG) describe + * relevant bits for the filter. + * + * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can + * filter for error frames (CAN_ERR_FLAG bit set in mask). For error frames + * there is a special filterlist and a special rx path filter handling. + * + * Return: + * Pointer to optimal filterlist for the given can_id/mask pair. + * Constistency checked mask. + * Reduced can_id to have a preprocessed filter compare value. + */ static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask, struct dev_rcv_lists *d) { canid_t inv = *can_id & CAN_INV_FILTER; /* save flag before masking */ - /* filter error frames */ + /* filter for error frames in extra filterlist */ if (*mask & CAN_ERR_FLAG) { - /* clear CAN_ERR_FLAG in list entry */ + /* clear CAN_ERR_FLAG in filter entry */ *mask &= CAN_ERR_MASK; return &d->rx[RX_ERR]; } - /* ensure valid values in can_mask */ - if (*mask & CAN_EFF_FLAG) - *mask &= (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG); - else - *mask &= (CAN_SFF_MASK | CAN_RTR_FLAG); + /* with cleared CAN_ERR_FLAG we have a simple mask/value filterpair */ + +#define CAN_EFF_RTR_FLAGS (CAN_EFF_FLAG | CAN_RTR_FLAG) + + /* ensure valid values in can_mask for 'SFF only' frame filtering */ + if ((*mask & CAN_EFF_FLAG) && !(*can_id & CAN_EFF_FLAG)) + *mask &= (CAN_SFF_MASK | CAN_EFF_RTR_FLAGS); /* reduce condition testing at receive time */ *can_id &= *mask; @@ -348,15 +377,19 @@ static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask, if (!(*mask)) return &d->rx[RX_ALL]; - /* use extra filterset for the subscription of exactly *ONE* can_id */ - if (*can_id & CAN_EFF_FLAG) { - if (*mask == (CAN_EFF_MASK | CAN_EFF_FLAG)) { - /* RFC: a use-case for hash-tables in the future? */ - return &d->rx[RX_EFF]; + /* extra filterlists for the subscription of a single non-RTR can_id */ + if (((*mask & CAN_EFF_RTR_FLAGS) == CAN_EFF_RTR_FLAGS) + && !(*can_id & CAN_RTR_FLAG)) { + + if (*can_id & CAN_EFF_FLAG) { + if (*mask == (CAN_EFF_MASK | CAN_EFF_RTR_FLAGS)) { + /* RFC: a future use-case for hash-tables? */ + return &d->rx[RX_EFF]; + } + } else { + if (*mask == (CAN_SFF_MASK | CAN_EFF_RTR_FLAGS)) + return &d->rx_sff[*can_id]; } - } else { - if (*mask == CAN_SFF_MASK) - return &d->rx_sff[*can_id]; } /* default: filter via can_id/can_mask */ diff --git a/net/can/bcm.c b/net/can/bcm.c index d0dd382..da0d426 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -64,10 +64,11 @@ #define BCM_CAN_DLC_MASK 0x0F /* clean private flags in can_dlc by masking */ /* get best masking value for can_rx_register() for a given single can_id */ -#define REGMASK(id) ((id & CAN_RTR_FLAG) | ((id & CAN_EFF_FLAG) ? \ - (CAN_EFF_MASK | CAN_EFF_FLAG) : CAN_SFF_MASK)) +#define REGMASK(id) ((id & CAN_EFF_FLAG) ? \ + (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \ + (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG)) -#define CAN_BCM_VERSION "20080415" +#define CAN_BCM_VERSION CAN_VERSION static __initdata const char banner[] = KERN_INFO "can: broadcast manager protocol (rev " CAN_BCM_VERSION ")\n";