From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6F770C282C3 for ; Thu, 24 Jan 2019 14:12:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3E10121855 for ; Thu, 24 Jan 2019 14:12:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="nSvliW7S" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728243AbfAXOMG (ORCPT ); Thu, 24 Jan 2019 09:12:06 -0500 Received: from vps0.lunn.ch ([185.16.172.187]:54026 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727562AbfAXOMF (ORCPT ); Thu, 24 Jan 2019 09:12:05 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Transfer-Encoding:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=M84i0JmOhDTK4EZbs3aoi3tkH3R5Qz8IfxN8OHMipOQ=; b=nSvliW7Se/+3HSgj1iHbHN7f+p WEOEDR7o82e7fgtxmubTvZR9b427xv3Vu8uCfDiptU1JUAFSjnnyvNxQB+LccxB8orVzkZtAVgpIl 3IERi3rnI7wPyZDpBJO+ktDD8OGJ7Qa22G+VQu237AeB9jbyj3doYi3uUQR1BfNSaKqg=; Received: from andrew by vps0.lunn.ch with local (Exim 4.89) (envelope-from ) id 1gmfjd-0008P4-Oz; Thu, 24 Jan 2019 15:12:01 +0100 Date: Thu, 24 Jan 2019 15:12:01 +0100 From: Andrew Lunn To: =?iso-8859-1?Q?Ren=E9?= van Dorst Cc: Florian Fainelli , Heiner Kallweit , "David S. Miller" , netdev@vger.kernel.org Subject: Re: [PATCH] sfp: sfp_read: split-up request when hw rx buffer is too small. Message-ID: <20190124141201.GE28903@lunn.ch> References: <20190123212046.13020-1-opensource@vdorst.com> <20190123213229.GA17772@lunn.ch> <20190124140323.Horde.-BXNAw3tfYO0-oHPatZeOOi@www.vdorst.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190124140323.Horde.-BXNAw3tfYO0-oHPatZeOOi@www.vdorst.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org > >>+ /* Many i2c hw have limited rx buffers, split-up request when needed. */ > >>+ while ((q->max_read_len) && (len > q->max_read_len)) { > >>+ ret = sfp->read(sfp, a2, addr, buf, q->max_read_len); > > > >Hi René > > > >I think you want to pass MIN(len, q->max_read_len) to read(). > > Hi Andrew, > > max_read_len is 0 when there is no quirk. > I can write it a bit differently depending on the outcome of my other email. Hi René No, you misunderstood me. > >>+ ret = sfp->read(sfp, a2, addr, buf, q->max_read_len); Say max_read_len = 64 The SFP code asks to read 68 bytes. The first call to read() is going to ask for 64 bytes. The second call is going to also ask for 64 bytes, writing 60 bytes passed the end of buf. Bad things then happen. > > >>+ if (ret < 0) > >>+ return ret; > >>+ rx_bytes += ret; > >>+ addr += q->max_read_len; > >>+ buf += q->max_read_len; > >>+ len -= q->max_read_len; > > > >I would prefer you add ret, not q->max_read_len. There is a danger it > >returned less bytes than you asked for. > > Getting less bytes then asked is already an error I think. > I could check the return size and directly return the number of bytes that I > have. The callers are checking for size and they can retry if wanted. So that > should not be an issue. If that is true, why is rx_bytes += ret, where as all the others are += q->max_read_len. Please be consistent. The general pattern of a read function in POSIX systems is that it returns how many bytes were actually returned. So i would always use += ret. > By reading the SSF spec we can write to a user writable EERPOM area of 120 > bytes. > But the current code has only has 1 sfp_write for a byte value. > > So for now I should say no. So how about adding a WARN_ON. If the request is bigger than what the quirk allows, make it very obvious we have an issue. Andrew