Date: Wed, 12 Nov 2008 11:13:07 +0900 From: Pyun YongHyeon <pyunyh@gmail.com> To: Dan Lukes <dan@obluda.cz> Cc: freebsd-bugs@FreeBSD.org, yongari@FreeBSD.org Subject: Re: kern/128766: [ PATCH ] VGE's [rt]xcsum can't be switched off Message-ID: <20081112021307.GA30203@cdnetworks.co.kr> In-Reply-To: <49197DB7.5080702@obluda.cz> References: <200811110127.mAB1RiQV095192@freefall.freebsd.org> <49194D6A.30807@obluda.cz> <20081111101606.GB26349@cdnetworks.co.kr> <49197DB7.5080702@obluda.cz>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Nov 11, 2008 at 01:42:31PM +0100, Dan Lukes wrote:
> 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.
>
Correct. That is one of reason why I check if_capabilities in
ioctl() without relying on ifhwioctl().(as you said ifhwioctl()
checks if_capabilities but I'd like more tight check due to other
part of kernel. Adding an additional if_capabilities check wouldn't
hurt and it's not in fast path.)
This also separates chipset specific code from capability ioctl.
Without that I would have embedded another variable into softc.
> >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.
>
[...]
> > > 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.
>
Ok. does the patch I posted work for you?
(I hoped I finish vge(4) cleanups but half-broken VT6122 I have
made it difficult to do that.)
--
Regards,
Pyun YongHyeon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081112021307.GA30203>