From patchwork Wed Jul 15 17:41:27 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Hershberger X-Patchwork-Id: 495916 X-Patchwork-Delegate: sjg@chromium.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 457321401AF for ; Thu, 16 Jul 2015 03:42:01 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=t6JoFLc6; dkim-atps=neutral Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id E25614B61F; Wed, 15 Jul 2015 19:41:55 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kTWFZKobs7d1; Wed, 15 Jul 2015 19:41:55 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id B3D6D4B616; Wed, 15 Jul 2015 19:41:54 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id B06634B616 for ; Wed, 15 Jul 2015 19:41:51 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 06pwuftDTDH1 for ; Wed, 15 Jul 2015 19:41:51 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-ig0-f173.google.com (mail-ig0-f173.google.com [209.85.213.173]) by theia.denx.de (Postfix) with ESMTPS id 4BF724B615 for ; Wed, 15 Jul 2015 19:41:48 +0200 (CEST) Received: by igcqs7 with SMTP id qs7so112803464igc.0 for ; Wed, 15 Jul 2015 10:41:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=Am6dyYGVTBdnbbkoYMfjOmSOsB+cx2drpJpWScG5joI=; b=t6JoFLc6Xr9tEKPWfyTheax05uHcG1w091tstjpQWTYoX3j0pPvvjqqrQGCup+IRW2 IXcFbh2xKmDlTDV5LWZf+mjXTrRy0xWbIpGiQ198fvTR7ZQZeezgd1N6JPlOVSyZhtCI tII9JN1/CaRBfAn8LA0hNIyXAXowhxYat5Ht4tdi/g7Z4Q0Nlp1jeOkCLMgalnkOnAyt 6C0x+YlcEk1MoSJk7lj0fw7R42x5j47hu02sA/yl/1wQOfNbwxAnxdPQqdOA4i8H57MP nbhJfF+9vu4UcwLzJl5QEpdAD6YSBqexxOKhNdYfBuNWJODKlUhG61ie3FxVWrYai7f0 5aSA== X-Received: by 10.107.156.203 with SMTP id f194mr6053778ioe.164.1436982106831; Wed, 15 Jul 2015 10:41:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.64.57.68 with HTTP; Wed, 15 Jul 2015 10:41:27 -0700 (PDT) In-Reply-To: <1436451351-13545-1-git-send-email-sjg@chromium.org> References: <1436451351-13545-1-git-send-email-sjg@chromium.org> From: Joe Hershberger Date: Wed, 15 Jul 2015 12:41:27 -0500 Message-ID: To: Simon Glass Cc: U-Boot Mailing List , Tom Rini Subject: Re: [U-Boot] [PATCH] RFC: dm: Add pointer checking for allocated data X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.15 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" Hi Simon, On Thu, Jul 9, 2015 at 9:15 AM, Simon Glass wrote: > With driver model drivers can have things stored in several places. There is > driver-private data, then the uclass can attach things to a device. If the > device is on a bus then its bus may attach parent data to the device too. > > At present everything is done through void pointers. It would be nice to > have a way to check that the correct C struct is used in each case. > > Here is a proposed implementation of a way of checking structures in driver > model. It relies on turning the existing dev_get_priv() function into a > macro which (if checking is enabled) checks that the structure names match. > Each xxx_auto_alloc_size turns into a structure containing a string (the > structure name) and the size. > > The dev_get_priv() macro has an extra parameter which is the structure being > accessed: > > struct eth_pdata *priv = dev_get_priv(dev, struct eth_pdata); > > and you get an error like this when things are wrong: > > Invalid access to device priv: dev=eth@10002000, expecting > 'struct eth_sandbox_priv', requested 'struct eth_pdata' This is interesting, but seems like something that could just be handled with good naming of structs. Type safety is great, but this is fairly heavy-handed way of doing it and only helps after hitting that code path in testing. Is there not a way to make the compiler help out? Maybe each uclass / driver should have wrapper functions that add type. Then the various uses all get type safety from the compiler and the driver only has to verify that the wrappers are correct. E.g.: > A new Kconfg option is added to turn this on, since it bloats the code a > little. > > The next step would be to extend it to all pointers in the device and > uclass. This is mostly a mechanical code change. > > Finally we should have a way of checking that the device pointer itself is > valid. For example if someone passes an invalid address like 0x12345 as the > 'struct udevice' then at present we will dutifully look for the devices' > driver and perform an invalid memory access. > > If we want to check for memory corruption one way would be to add a magic > numnber before each allocated memory area. Perhaps this is more the role > of dlmalloc(), but if required it could be attached to Masahiro's devres > feature, which already prepends some data to every allocated area. It seems like it should be the more generic approach. > Comments welcome. I'd like to figure this out soon as it involves trivial > but invasive patches to change each driver. > > Signed-off-by: Simon Glass Cheers, -Joe diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c index 4e083d3..b01ae0c 100644 --- a/drivers/net/sandbox.c +++ b/drivers/net/sandbox.c @@ -33,6 +33,11 @@ struct eth_sandbox_priv { static bool disabled[8] = {false}; static bool skip_timeout; +static struct eth_sandbox_priv *eth_sandbox_get_priv(struct udevice *dev) +{ + return dev_get_priv(dev); +} + /* * sandbox_eth_disable_response() * @@ -56,7 +61,7 @@ void sandbox_eth_skip_timeout(void) static int sb_eth_start(struct udevice *dev) { - struct eth_sandbox_priv *priv = dev_get_priv(dev); + struct eth_sandbox_priv *priv = sandbox_eth_get_priv(dev); debug("eth_sandbox: Start\n");