From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755415AbbBTTAa (ORCPT ); Fri, 20 Feb 2015 14:00:30 -0500 Received: from mail-wg0-f42.google.com ([74.125.82.42]:61678 "EHLO mail-wg0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754613AbbBTTA0 (ORCPT ); Fri, 20 Feb 2015 14:00:26 -0500 Message-ID: <54E78446.8050800@linaro.org> Date: Fri, 20 Feb 2015 19:00:22 +0000 From: Srinivas Kandagatla User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Russell King - ARM Linux CC: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, Arnd Bergmann , Pawel Moll , Greg Kroah-Hartman , linux-api@vger.kernel.org, broonie@kernel.org, Stephen Boyd , linux-kernel@vger.kernel.org, Rob Herring , Kumar Gala , Maxime Ripard Subject: Re: [RFC PATCH 1/3] eeprom: Add a simple EEPROM framework References: <1424365639-26634-1-git-send-email-srinivas.kandagatla@linaro.org> <1424365708-26681-1-git-send-email-srinivas.kandagatla@linaro.org> <20150220174641.GI8656@n2100.arm.linux.org.uk> In-Reply-To: <20150220174641.GI8656@n2100.arm.linux.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20/02/15 17:46, Russell King - ARM Linux wrote: > On Thu, Feb 19, 2015 at 05:08:28PM +0000, Srinivas Kandagatla wrote: >> +static struct class eeprom_class = { >> + .name = "eeprom", >> + .dev_groups = eeprom_dev_groups, >> +}; >> + >> +int eeprom_register(struct eeprom_device *eeprom) >> +{ >> + int rval; >> + >> + if (!eeprom->regmap || !eeprom->size) { >> + dev_err(eeprom->dev, "Regmap not found\n"); >> + return -EINVAL; >> + } >> + >> + eeprom->id = ida_simple_get(&eeprom_ida, 0, 0, GFP_KERNEL); >> + if (eeprom->id < 0) >> + return eeprom->id; >> + >> + eeprom->edev.class = &eeprom_class; >> + eeprom->edev.parent = eeprom->dev; >> + eeprom->edev.of_node = eeprom->dev ? eeprom->dev->of_node : NULL; >> + dev_set_name(&eeprom->edev, "eeprom%d", eeprom->id); >> + >> + device_initialize(&eeprom->edev); >> + >> + dev_dbg(&eeprom->edev, "Registering eeprom device %s\n", >> + dev_name(&eeprom->edev)); >> + >> + rval = device_add(&eeprom->edev); >> + if (rval) >> + return rval; >> + >> + mutex_lock(&eeprom_list_mutex); >> + list_add(&eeprom->list, &eeprom_list); >> + mutex_unlock(&eeprom_list_mutex); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(eeprom_register); >> + >> +int eeprom_unregister(struct eeprom_device *eeprom) >> +{ >> + device_del(&eeprom->edev); >> + >> + mutex_lock(&eeprom_list_mutex); >> + list_del(&eeprom->list); >> + mutex_unlock(&eeprom_list_mutex); >> + >> + return 0; > > There's problems with this. "edev" is embedded into eeprom, and "edev" > is a refcounted structure - it has a lifetime, and the lifetime of > eeprom is thus determined by edev. > > What this means is that calling eeprom_unregister() and then freeing > the eeprom structure is a potentially unsafe operation - the memory > backing edev must only be freed when the release method for the > struct device has been called. Thats a good catch, Yes I see the issue. Moving the struct eeprom_device allocation to eeprom core.c should address this issue. This makes eeprom self contained and can safely free the eeprom_device memory in eeprom_class.eeprom_release(). Will fix this in next version. > >> +struct eeprom_device { >> + struct regmap *regmap; >> + int stride; >> + size_t size; >> + struct device *dev; > > Failing to understand and address the driver model life time issues is > a major blocker for this patch. Sorry. >