From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757143Ab2GKRwb (ORCPT ); Wed, 11 Jul 2012 13:52:31 -0400 Received: from mail-lb0-f174.google.com ([209.85.217.174]:50665 "EHLO mail-lb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756588Ab2GKRw1 (ORCPT ); Wed, 11 Jul 2012 13:52:27 -0400 MIME-Version: 1.0 In-Reply-To: <4FFD1FE7.6010504@huawei.com> References: <1341935655-5381-1-git-send-email-jiang.liu@huawei.com> <1341935655-5381-6-git-send-email-jiang.liu@huawei.com> <4FFCEDDE.2080907@huawei.com> <4FFD1FE7.6010504@huawei.com> From: Bjorn Helgaas Date: Wed, 11 Jul 2012 11:52:05 -0600 Message-ID: Subject: Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences To: Jiang Liu Cc: Jiang Liu , Don Dutile , Yinghai Lu , Taku Izumi , "Rafael J . Wysocki" , Kenji Kaneshige , Yijing Wang , Keping Chen , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jul 11, 2012 at 12:40 AM, Jiang Liu wrote: > On 2012-7-11 11:40, Bjorn Helgaas wrote: > >>> Good point. Return success when reading unimplemented registeres, that >>> may simplify code. For we still should return -EINVAL when writing >>> unimplemented registers, right? >> >> Yeah, I guess it's OK to return -EINVAL when *writing* to an >> unimplemented register. Hopefully the caller is structured such that >> we don't even try to write in that case. It'd be interesting to audit >> the callers and explore that, but I haven't done that. > Hi Bjorn, > Seems it would be better to return error code for unimplemented > registers, otherwise following code will becomes more complex. A special > error code for unimplemented registers, such as -EIO? I think you're asking about returning error for *reads* of unimplemented registers? I guess I still think it's OK to completely hide the v1 nastiness inside these accessors, and return success with a zero value when reading. Having several different error returns seems like overkill for this case. Nobody wants to distinguish between different reasons for failure. I'm actually not sure that it's worth returning an error even when *writing* an unimplemented register. What if we return success and just drop the write? Maybe these should even be void functions. It feels like the only real use of the return value is to detect programmer error, and I don't think that's very effective. If we remove the return values, people will have to focus on the *data*, which seems more important anyway. > static void rtl_disable_clock_request(struct pci_dev *pdev) > { > u16 ctl; > > if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl)) { > ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN; > pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl); > } > } I would write that as: if (!pci_is_pcie(pdev) return; pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, &ctl); if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN) pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl & ~PCI_EXP_LNKCTL_CLKREQ_EN); which does the right thing regardless of what we do for return values, and saves a config write in the case where LNKCTL is implemented and CLKREQ_EN is already cleared. Bjorn