Hi Oliver, Thanks for the patch. > -----Original Message----- > From: Oliver Hartkopp [mailto:socketcan@xxxxxxxxxxxx] > Sent: 08 March 2016 17:14 > To: linux-can@xxxxxxxxxxxxxxx > Cc: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@xxxxxxxxxxxxxx>; > Oliver Hartkopp <socketcan@xxxxxxxxxxxx>; linux-stable 3 . 16+ > <stable@xxxxxxxxxxxxxxx> > Subject: [RFC] [PATCH] can: fix handling of unmodifiable configuration > options > > As described in 'can: m_can: tag current CAN FD controllers as non-ISO' > (6cfda7fbebe) it is possible to define fixed configuration options by > setting the according bit in 'ctrlmode' and clear it in > 'ctrlmode_supported'. Shouldn't we document this? A comment to these variable definitions perhaps? > This leads to the incovenience that the fixed configuration can not be > passed by netlink (ip tool) even when it has the correct value (e.g. non- > ISO, FD). > > This patch fixes that issue and allows fixed set bit values to be set > again which does not change the unmodifiable content. > > Additionally it fixes the inconsitency that was prohibiting the support of > 'CANFD-only' controller drivers. > > Reported-by: Ramesh Shanmugasundaram > <ramesh.shanmugasundaram@xxxxxxxxxxxxxx> > Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx> > Cc: linux-stable <stable@xxxxxxxxxxxxxxx> 3.16+ > --- > drivers/net/can/dev.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index > 141c2a4..b084cfd 100644 > --- a/drivers/net/can/dev.c > +++ b/drivers/net/can/dev.c > @@ -696,11 +696,18 @@ int can_change_mtu(struct net_device *dev, int > new_mtu) > /* allow change of MTU according to the CANFD ability of the device > */ > switch (new_mtu) { > case CAN_MTU: > + /* take care of 'CANFD-only' controllers */ > + if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) && > + (priv->ctrlmode & CAN_CTRLMODE_FD)) > + return -EINVAL; > + > priv->ctrlmode &= ~CAN_CTRLMODE_FD; > break; > > case CANFD_MTU: > - if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD)) > + /* check for CANFD capability */ > + if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD) && > + !(priv->ctrlmode & CAN_CTRLMODE_FD)) > return -EINVAL; > > priv->ctrlmode |= CAN_CTRLMODE_FD; > @@ -819,10 +826,13 @@ static int can_changelink(struct net_device *dev, > return -EBUSY; > cm = nla_data(data[IFLA_CAN_CTRLMODE]); > > - /* check whether changed bits are allowed to be modified */ > - if (cm->mask & ~priv->ctrlmode_supported) > + /* check whether provided bits are allowed to be passed */ > + if (cm->mask & ~(priv->ctrlmode_supported | priv->ctrlmode)) > return -EOPNOTSUPP; I think we need to separate this check into two. Otherwise, it will still return success for "fd off" config even though the configuration is not really applied. > > + /* reduce to bits that are allowed to be modified */ > + cm->mask &= priv->ctrlmode_supported; If we separate the above check, I think we don't have to do this. Thanks, Ramesh -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html