Thu, Jun 11, 2015 at 05:03:21PM CEST, sfel...@gmail.com wrote: >On Wed, Jun 10, 2015 at 11:16 PM, Jiri Pirko <j...@resnulli.us> wrote: >> Thu, Jun 11, 2015 at 12:00:47AM CEST, sfel...@gmail.com wrote: >>>On Wed, Jun 10, 2015 at 2:47 PM, Scott Feldman <sfel...@gmail.com> wrote: >>>> On Wed, Jun 10, 2015 at 2:25 PM, David Ahern <dsah...@gmail.com> wrote: >>>>> On 6/10/15 2:56 PM, sfel...@gmail.com wrote: >>>>>> >>>>>> From: Scott Feldman <sfel...@gmail.com> >>>>>> >>>>>> Fix a BUG() where CONFIG_NET_SWITCHDEV is set but the driver for a >>>>>> bridged >>>>>> port does not support switchdec_port_attr_set op. Don't BUG() if >>>>>> -EOPNOTSUPP is returned. >>>>>> >>>>>> Signed-off-by: Scott Feldman <sfel...@gmail.com> >>>>>> Reported-by: Brenden Blanco <bbla...@plumgrid.com> >>>>>> --- >>>>>> net/switchdev/switchdev.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c >>>>>> index e008057..99bced4 100644 >>>>>> --- a/net/switchdev/switchdev.c >>>>>> +++ b/net/switchdev/switchdev.c >>>>>> @@ -103,7 +103,7 @@ static void switchdev_port_attr_set_work(struct >>>>>> work_struct *work) >>>>>> >>>>>> rtnl_lock(); >>>>>> err = switchdev_port_attr_set(asw->dev, &asw->attr); >>>>>> - BUG_ON(err); >>>>>> + BUG_ON(err && err != -EOPNOTSUPP); >>>>>> rtnl_unlock(); >>>>>> >>>>>> dev_put(asw->dev); >>>>>> >>>>> >>>>> Should that be WARN_ON instead of BUG_ON? >>>> >>>> I think I had it as WARN when we were working on the initial patches, >>>> but we changed it to BUG_ON because we should only get an error here >>>> if the driver screwed something up between PREPARE phase and COMMIT >>>> phase, so it should be considered a driver bug which needs fixing. >>> >>>Actually, ignore what I said above. I was confusing this BUG_ON with >>>the one in switchdev_port_attr_set(). Perhaps this BUG_ON() you're >>>commenting on should be WARN(). A driver could return an err in >>>PREPARE phase and that shouldn't be a BUG_ON situation; seems WARN >>>would be better. It the case where the driver returns an err in >>>COMMIT phase but didn't return an err in PREPARE phase we want to >>>BUG_ON(). Maybe that case doesn't justify BUG_ON either, based on the >>>link you posted. >>> >>>Jiri, IIRC, you suggested the BUG_ON(). Does it still sound right >>>based on the point David is raising? >> >> Hmm, looking at code of switchdev_port_attr_set. In case that fails in >> prepare state (which can easily happen for example due to -ENOMEM) this >> BUG_ON is hit as well. That is not right. I think we should change it >> just to warning. Also I think that prink (or a flavour) is more suitable >> here than WARN. > >Thanks, I'll change it to netdev_err. > >> Btw, why switchdev_port_obj_add has WARN and not BUG ? > >Not sure. We should be consistent. WARN seems better for both >obj_add/attr_set than BUG, given the link David Ahern posted. Do you >agree?
Okay. Sounds reasonable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
