Date: Tue, 11 Nov 2008 13:42:31 +0100 From: Dan Lukes <dan@obluda.cz> To: pyunyh@gmail.com Cc: freebsd-bugs@FreeBSD.org, yongari@FreeBSD.org Subject: Re: kern/128766: [ PATCH ] VGE's [rt]xcsum can't be switched off Message-ID: <49197DB7.5080702@obluda.cz> In-Reply-To: <20081111101606.GB26349@cdnetworks.co.kr> References: <200811110127.mAB1RiQV095192@freefall.freebsd.org> <49194D6A.30807@obluda.cz> <20081111101606.GB26349@cdnetworks.co.kr>
next in thread | previous in thread | raw e-mail | index | archive | help
Pyun YongHyeon napsal/wrote, On 11/11/08 11:16:
> > The tests (ifp->if_capabilities & IFCAP_TXCSUM) and
> > (ifp->if_capabilities & IFCAP_RXCSUM) is true everytime because the test
> > is done already in upper layer (see net.c:ifhwioctl(), it return EINVAL
> > when someone try to set capabilities not offered by driver)
> >
> > Code with such tests removed ...
> >
> > ===================================================================
> > if (mask & IFCAP_TXCSUM) != 0) {
> > ifp->if_capenable ^= IFCAP_TXCSUM;
> > if ((ifp->if_capenable & IFCAP_TXCSUM) != 0)
> > ifp->if_hwassist |= VGE_CSUM_FEATURES;
> > else
> > ifp->if_hwassist &= ~VGE_CSUM_FEATURES;
> > }
> > if (mask & IFCAP_RXCSUM != 0)
> > ifp->if_capenable ^= IFCAP_RXCSUM;
> > }
> > ===================================================================
> >
>
> Yeah, ATM your code is correct for userland side. But there is no
> guarantee that all vge(4) hardwares have the same capabilities.
It check it against if_capabilites, eg against capabilites supported by
a instance of driver as if_capabilities are capabilities of such
particular hardware.
> If
> Tx checksum offload was broken for specific revision, driver should
> check whether the capability is available against the hardware.
If txcsum was broken for specific revision then driver shall not mark
the txcsum to be supported in if_capabilities.
> There are still a couple of drivers in tree that misuses this kind
> of checking.
> Also remember some subsystem still directly calls driver ioctls
> without relying on ifhwioctl() so it still needs the check
> (e.g. bridge(4)).
True. Either drivers or callers shall do such test. It's questionable if
all drivers shall be corrected or bridge(4) code, but you are comitter,
it's your decision.
>
> > ... is OK but ~VGE_CSUM_FEATURES shall be ~VGE_CSUM_FEATURE instead
> > (-VGE_CSUM_FEATURES may not be equal to ~VGE_CSUM_FEATURES althought it
> > is on most platforms).
> >
>
> Sorry I don't see the point. vge(4) supports Tx IP/TCP/UDP checksum
> offload and it's just represention of the feature in driver. Why do
> you need to specify -VGE_CSUM_FEATURES?
Forged it.
When I looked into your patch, the character encoding used by my browser
has been ISO-8859-2 and the line
+ ifp->if_hwassist &= ~VGE_CSUM_FEATURES;
has been shown as
+ ifp->if_hwassist &= -VGE_CSUM_FEATURES;
Thus I noticed the ~VGE_CSUM_FEATURES shall be used instead. Ignore such
notice.
> > If we shall do
> > mask ^= IFCAP_POLLING
> > on the end of IFCAP_POLLING handling and
> > mask ^= (IFCAP_TXCSUM | IFCAP_RXCSUM)
> > on the end of CSUM handling then we can do
> >
> > if (mask != 0)
> > return(ENOTSUPP)
> > on the end of SIOCSIFCAP.
> >
>
> See above. I think you can't do that. Drivers should always check
> against its usable capabilities.
I'm speaking about something diferent. if_capabilities are capabilities
for such instance of driver. But i'm not speaking about the request to
change of capability that are not supported by driver at all. I'm
speaking about the request to change the capability that is supported
but can't be turned off.
> And I'm not sure ENOTSUPP is
> right error code. I guess if ioctl itself is not available,
> ENOTSUPP would be correct one otherwise it can return EINVAL for
> handled (invalid) ioctl call, ENOTTY for unhandled ioctl call.
OK
Dan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?49197DB7.5080702>