From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg KH Subject: Re: [PATCH v3 2/2] Embedded USB Debugger (EUD) driver Date: Thu, 15 Nov 2018 15:09:47 -0800 Message-ID: <20181115230947.GC26568@kroah.com> References: <1542310374-18474-1-git-send-email-pheragu@codeaurora.org> <1542310374-18474-3-git-send-email-pheragu@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1542310374-18474-3-git-send-email-pheragu@codeaurora.org> Sender: linux-kernel-owner@vger.kernel.org To: Prakruthi Deepak Heragu Cc: linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, ckadabi@codeaurora.org, tsoni@codeaurora.org, bryanh@codeaurora.org, psodagud@codeaurora.org, rnayak@codeaurora.org, Satya Durga Srinivasu Prabhala List-Id: linux-arm-msm@vger.kernel.org On Thu, Nov 15, 2018 at 11:32:54AM -0800, Prakruthi Deepak Heragu wrote: > +struct device_attribute eud_attribute = { > + .attr.name = "enable", > + .attr.mode = 0644, > + .show = eud_enable_show, > + .store = eud_enable_store, > +}; Please use: static DEVICE_ATTR_RW(enable); instead of open-coding all of that mess. Also it makes it static, this should not be a global symbol. > + > +static void eud_event_notifier(struct work_struct *eud_work) > +{ > + struct eud_chip *chip = container_of(eud_work, struct eud_chip, > + eud_work); > + int ret; > + > + if (chip->int_status == EUD_INT_VBUS) { > + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id, > + chip->usb_attach); > + if (ret) > + return; > + } else if (chip->int_status == EUD_INT_CHGR) { > + ret = extcon_set_state_sync(chip->extcon, chip->extcon_id, > + chip->chgr_enable); > + if (ret) > + return; > + } > +} > + > +static void usb_attach_detach(struct eud_chip *chip) > +{ > + u32 reg; > + > + chip->extcon_id = EXTCON_USB; > + /* read ctl_out_1[4] to find USB attach or detach event */ > + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1); > + if (reg & BIT(4)) > + chip->usb_attach = true; > + else > + chip->usb_attach = false; > + > + schedule_work(&chip->eud_work); > + > + /* set and clear vbus_int_clr[0] to clear interrupt */ > + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR); > + /* Ensure Register Writes Complete */ > + wmb(); > + writel_relaxed(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR); > +} > + > +static void chgr_enable_disable(struct eud_chip *chip) > +{ > + u32 reg; > + > + chip->extcon_id = EXTCON_CHG_USB_SDP; > + /* read ctl_out_1[6] to find charger enable or disable event */ > + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_CTL_OUT_1); > + if (reg & BIT(6)) > + chip->chgr_enable = true; > + else > + chip->chgr_enable = false; > + > + schedule_work(&chip->eud_work); > + > + /* set and clear chgr_int_clr[0] to clear interrupt */ > + writel_relaxed(BIT(0), chip->eud_reg_base + EUD_REG_CHGR_INT_CLR); > + /* Ensure Register Writes Complete */ > + wmb(); > + writel_relaxed(0, chip->eud_reg_base + EUD_REG_CHGR_INT_CLR); > +} > + > +static void pet_eud(struct eud_chip *chip) > +{ > + u32 reg; > + > + /* read sw_attach_det[0] to find attach/detach event */ > + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET); > + if (reg & BIT(0)) { > + /* Detach & Attach pet for EUD */ > + writel_relaxed(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET); > + /* Ensure Register Writes Complete */ > + wmb(); > + /* Delay to make sure detach pet is done before attach pet */ > + udelay(100); > + writel_relaxed(BIT(0), chip->eud_reg_base + > + EUD_REG_SW_ATTACH_DET); > + /* Ensure Register Writes Complete */ > + wmb(); > + } else { > + /* Attach pet for EUD */ > + writel_relaxed(BIT(0), chip->eud_reg_base + > + EUD_REG_SW_ATTACH_DET); > + /* Ensure Register Writes Complete */ > + wmb(); > + } > +} > + > +static irqreturn_t handle_eud_irq(int irq, void *data) > +{ > + struct eud_chip *chip = data; > + u32 reg; > + > + /* read status register and find out which interrupt triggered */ > + reg = readl_relaxed(chip->eud_reg_base + EUD_REG_INT_STATUS_1); > + switch (reg & EUD_INT_ALL) { > + case EUD_INT_VBUS: > + chip->int_status = EUD_INT_VBUS; > + usb_attach_detach(chip); > + break; > + case EUD_INT_CHGR: > + chip->int_status = EUD_INT_CHGR; > + chgr_enable_disable(chip); > + break; > + case EUD_INT_SAFE_MODE: > + pet_eud(chip); > + break; > + default: > + return IRQ_NONE; > + } > + return IRQ_HANDLED; > +} > + > +static int msm_eud_probe(struct platform_device *pdev) > +{ > + struct eud_chip *chip; > + struct resource *res; > + int ret; > + > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + chip->dev = &pdev->dev; > + platform_set_drvdata(pdev, chip); > + > + ret = device_create_file(&pdev->dev, &eud_attribute); > + if (ret) > + return ret; You just raced with userspace and lost :( Isn't there a way to add the attribute to the platform device as a group to have it correctly added by the driver core? Also, you are adding an attribute to a structure that you do not control/own, are you _sure_ you want to do that? It's a bit odd and you don't really control the lifetime of that device. > + > + chip->extcon = devm_extcon_dev_allocate(&pdev->dev, eud_extcon_cable); > + if (IS_ERR(chip->extcon)) > + return PTR_ERR(chip->extcon); As an example of this problem, the sysfs file is now there if this was an error. Yet that sysfs file now means nothing and if userspace touches it bad things will happen. So please at the very least, properly clean up your error paths. And look into doing this correctly. thanks, greg k-h From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v3,2/2] Embedded USB Debugger (EUD) driver From: Greg KH Message-Id: <20181115230947.GC26568@kroah.com> Date: Thu, 15 Nov 2018 15:09:47 -0800 To: Prakruthi Deepak Heragu Cc: linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, ckadabi@codeaurora.org, tsoni@codeaurora.org, bryanh@codeaurora.org, psodagud@codeaurora.org, rnayak@codeaurora.org, Satya Durga Srinivasu Prabhala List-ID: T24gVGh1LCBOb3YgMTUsIDIwMTggYXQgMTE6MzI6NTRBTSAtMDgwMCwgUHJha3J1dGhpIERlZXBh ayBIZXJhZ3Ugd3JvdGU6Cj4gK3N0cnVjdCBkZXZpY2VfYXR0cmlidXRlIGV1ZF9hdHRyaWJ1dGUg PSB7Cj4gKwkuYXR0ci5uYW1lID0gImVuYWJsZSIsCj4gKwkuYXR0ci5tb2RlID0gMDY0NCwKPiAr CS5zaG93ID0gZXVkX2VuYWJsZV9zaG93LAo+ICsJLnN0b3JlID0gZXVkX2VuYWJsZV9zdG9yZSwK PiArfTsKClBsZWFzZSB1c2U6CglzdGF0aWMgREVWSUNFX0FUVFJfUlcoZW5hYmxlKTsKaW5zdGVh ZCBvZiBvcGVuLWNvZGluZyBhbGwgb2YgdGhhdCBtZXNzLgoKQWxzbyBpdCBtYWtlcyBpdCBzdGF0 aWMsIHRoaXMgc2hvdWxkIG5vdCBiZSBhIGdsb2JhbCBzeW1ib2wuCgo+ICsKPiArc3RhdGljIHZv aWQgZXVkX2V2ZW50X25vdGlmaWVyKHN0cnVjdCB3b3JrX3N0cnVjdCAqZXVkX3dvcmspCj4gK3sK PiArCXN0cnVjdCBldWRfY2hpcCAqY2hpcCA9IGNvbnRhaW5lcl9vZihldWRfd29yaywgc3RydWN0 IGV1ZF9jaGlwLAo+ICsJCQkJCWV1ZF93b3JrKTsKPiArCWludCByZXQ7Cj4gKwo+ICsJaWYgKGNo aXAtPmludF9zdGF0dXMgPT0gRVVEX0lOVF9WQlVTKSB7Cj4gKwkJcmV0ID0gZXh0Y29uX3NldF9z dGF0ZV9zeW5jKGNoaXAtPmV4dGNvbiwgY2hpcC0+ZXh0Y29uX2lkLAo+ICsJCQkJCWNoaXAtPnVz Yl9hdHRhY2gpOwo+ICsJCWlmIChyZXQpCj4gKwkJCXJldHVybjsKPiArCX0gZWxzZSBpZiAoY2hp cC0+aW50X3N0YXR1cyA9PSBFVURfSU5UX0NIR1IpIHsKPiArCQlyZXQgPSBleHRjb25fc2V0X3N0 YXRlX3N5bmMoY2hpcC0+ZXh0Y29uLCBjaGlwLT5leHRjb25faWQsCj4gKwkJCQkJY2hpcC0+Y2hn cl9lbmFibGUpOwo+ICsJCWlmIChyZXQpCj4gKwkJCXJldHVybjsKPiArCX0KPiArfQo+ICsKPiAr c3RhdGljIHZvaWQgdXNiX2F0dGFjaF9kZXRhY2goc3RydWN0IGV1ZF9jaGlwICpjaGlwKQo+ICt7 Cj4gKwl1MzIgcmVnOwo+ICsKPiArCWNoaXAtPmV4dGNvbl9pZCA9IEVYVENPTl9VU0I7Cj4gKwkv KiByZWFkIGN0bF9vdXRfMVs0XSB0byBmaW5kIFVTQiBhdHRhY2ggb3IgZGV0YWNoIGV2ZW50ICov Cj4gKwlyZWcgPSByZWFkbF9yZWxheGVkKGNoaXAtPmV1ZF9yZWdfYmFzZSArIEVVRF9SRUdfQ1RM X09VVF8xKTsKPiArCWlmIChyZWcgJiBCSVQoNCkpCj4gKwkJY2hpcC0+dXNiX2F0dGFjaCA9IHRy dWU7Cj4gKwllbHNlCj4gKwkJY2hpcC0+dXNiX2F0dGFjaCA9IGZhbHNlOwo+ICsKPiArCXNjaGVk dWxlX3dvcmsoJmNoaXAtPmV1ZF93b3JrKTsKPiArCj4gKwkvKiBzZXQgYW5kIGNsZWFyIHZidXNf aW50X2NsclswXSB0byBjbGVhciBpbnRlcnJ1cHQgKi8KPiArCXdyaXRlbF9yZWxheGVkKEJJVCgw KSwgY2hpcC0+ZXVkX3JlZ19iYXNlICsgRVVEX1JFR19WQlVTX0lOVF9DTFIpOwo+ICsJLyogRW5z dXJlIFJlZ2lzdGVyIFdyaXRlcyBDb21wbGV0ZSAqLwo+ICsJd21iKCk7Cj4gKwl3cml0ZWxfcmVs YXhlZCgwLCBjaGlwLT5ldWRfcmVnX2Jhc2UgKyBFVURfUkVHX1ZCVVNfSU5UX0NMUik7Cj4gK30K PiArCj4gK3N0YXRpYyB2b2lkIGNoZ3JfZW5hYmxlX2Rpc2FibGUoc3RydWN0IGV1ZF9jaGlwICpj aGlwKQo+ICt7Cj4gKwl1MzIgcmVnOwo+ICsKPiArCWNoaXAtPmV4dGNvbl9pZCA9IEVYVENPTl9D SEdfVVNCX1NEUDsKPiArCS8qIHJlYWQgY3RsX291dF8xWzZdIHRvIGZpbmQgY2hhcmdlciBlbmFi bGUgb3IgZGlzYWJsZSBldmVudCAqLwo+ICsJcmVnID0gcmVhZGxfcmVsYXhlZChjaGlwLT5ldWRf cmVnX2Jhc2UgKyBFVURfUkVHX0NUTF9PVVRfMSk7Cj4gKwlpZiAocmVnICYgQklUKDYpKQo+ICsJ CWNoaXAtPmNoZ3JfZW5hYmxlID0gdHJ1ZTsKPiArCWVsc2UKPiArCQljaGlwLT5jaGdyX2VuYWJs ZSA9IGZhbHNlOwo+ICsKPiArCXNjaGVkdWxlX3dvcmsoJmNoaXAtPmV1ZF93b3JrKTsKPiArCj4g KwkvKiBzZXQgYW5kIGNsZWFyIGNoZ3JfaW50X2NsclswXSB0byBjbGVhciBpbnRlcnJ1cHQgKi8K PiArCXdyaXRlbF9yZWxheGVkKEJJVCgwKSwgY2hpcC0+ZXVkX3JlZ19iYXNlICsgRVVEX1JFR19D SEdSX0lOVF9DTFIpOwo+ICsJLyogRW5zdXJlIFJlZ2lzdGVyIFdyaXRlcyBDb21wbGV0ZSAqLwo+ ICsJd21iKCk7Cj4gKwl3cml0ZWxfcmVsYXhlZCgwLCBjaGlwLT5ldWRfcmVnX2Jhc2UgKyBFVURf UkVHX0NIR1JfSU5UX0NMUik7Cj4gK30KPiArCj4gK3N0YXRpYyB2b2lkIHBldF9ldWQoc3RydWN0 IGV1ZF9jaGlwICpjaGlwKQo+ICt7Cj4gKwl1MzIgcmVnOwo+ICsKPiArCS8qIHJlYWQgc3dfYXR0 YWNoX2RldFswXSB0byBmaW5kIGF0dGFjaC9kZXRhY2ggZXZlbnQgKi8KPiArCXJlZyA9IHJlYWRs X3JlbGF4ZWQoY2hpcC0+ZXVkX3JlZ19iYXNlICsgRVVEX1JFR19TV19BVFRBQ0hfREVUKTsKPiAr CWlmIChyZWcgJiBCSVQoMCkpIHsKPiArCQkvKiBEZXRhY2ggJiBBdHRhY2ggcGV0IGZvciBFVUQg Ki8KPiArCQl3cml0ZWxfcmVsYXhlZCgwLCBjaGlwLT5ldWRfcmVnX2Jhc2UgKyBFVURfUkVHX1NX X0FUVEFDSF9ERVQpOwo+ICsJCS8qIEVuc3VyZSBSZWdpc3RlciBXcml0ZXMgQ29tcGxldGUgKi8K PiArCQl3bWIoKTsKPiArCQkvKiBEZWxheSB0byBtYWtlIHN1cmUgZGV0YWNoIHBldCBpcyBkb25l IGJlZm9yZSBhdHRhY2ggcGV0ICovCj4gKwkJdWRlbGF5KDEwMCk7Cj4gKwkJd3JpdGVsX3JlbGF4 ZWQoQklUKDApLCBjaGlwLT5ldWRfcmVnX2Jhc2UgKwo+ICsJCQkJCUVVRF9SRUdfU1dfQVRUQUNI X0RFVCk7Cj4gKwkJLyogRW5zdXJlIFJlZ2lzdGVyIFdyaXRlcyBDb21wbGV0ZSAqLwo+ICsJCXdt YigpOwo+ICsJfSBlbHNlIHsKPiArCQkvKiBBdHRhY2ggcGV0IGZvciBFVUQgKi8KPiArCQl3cml0 ZWxfcmVsYXhlZChCSVQoMCksIGNoaXAtPmV1ZF9yZWdfYmFzZSArCj4gKwkJCQkJRVVEX1JFR19T V19BVFRBQ0hfREVUKTsKPiArCQkvKiBFbnN1cmUgUmVnaXN0ZXIgV3JpdGVzIENvbXBsZXRlICov Cj4gKwkJd21iKCk7Cj4gKwl9Cj4gK30KPiArCj4gK3N0YXRpYyBpcnFyZXR1cm5fdCBoYW5kbGVf ZXVkX2lycShpbnQgaXJxLCB2b2lkICpkYXRhKQo+ICt7Cj4gKwlzdHJ1Y3QgZXVkX2NoaXAgKmNo aXAgPSBkYXRhOwo+ICsJdTMyIHJlZzsKPiArCj4gKwkvKiByZWFkIHN0YXR1cyByZWdpc3RlciBh bmQgZmluZCBvdXQgd2hpY2ggaW50ZXJydXB0IHRyaWdnZXJlZCAqLwo+ICsJcmVnID0gcmVhZGxf cmVsYXhlZChjaGlwLT5ldWRfcmVnX2Jhc2UgKyBFVURfUkVHX0lOVF9TVEFUVVNfMSk7Cj4gKwlz d2l0Y2ggKHJlZyAmIEVVRF9JTlRfQUxMKSB7Cj4gKwljYXNlIEVVRF9JTlRfVkJVUzoKPiArCQlj aGlwLT5pbnRfc3RhdHVzID0gRVVEX0lOVF9WQlVTOwo+ICsJCXVzYl9hdHRhY2hfZGV0YWNoKGNo aXApOwo+ICsJCWJyZWFrOwo+ICsJY2FzZSBFVURfSU5UX0NIR1I6Cj4gKwkJY2hpcC0+aW50X3N0 YXR1cyA9IEVVRF9JTlRfQ0hHUjsKPiArCQljaGdyX2VuYWJsZV9kaXNhYmxlKGNoaXApOwo+ICsJ CWJyZWFrOwo+ICsJY2FzZSBFVURfSU5UX1NBRkVfTU9ERToKPiArCQlwZXRfZXVkKGNoaXApOwo+ ICsJCWJyZWFrOwo+ICsJZGVmYXVsdDoKPiArCQlyZXR1cm4gSVJRX05PTkU7Cj4gKwl9Cj4gKwly ZXR1cm4gSVJRX0hBTkRMRUQ7Cj4gK30KPiArCj4gK3N0YXRpYyBpbnQgbXNtX2V1ZF9wcm9iZShz dHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KQo+ICt7Cj4gKwlzdHJ1Y3QgZXVkX2NoaXAgKmNo aXA7Cj4gKwlzdHJ1Y3QgcmVzb3VyY2UgKnJlczsKPiArCWludCByZXQ7Cj4gKwo+ICsJY2hpcCA9 IGRldm1fa3phbGxvYygmcGRldi0+ZGV2LCBzaXplb2YoKmNoaXApLCBHRlBfS0VSTkVMKTsKPiAr CWlmICghY2hpcCkKPiArCQlyZXR1cm4gLUVOT01FTTsKPiArCj4gKwljaGlwLT5kZXYgPSAmcGRl di0+ZGV2Owo+ICsJcGxhdGZvcm1fc2V0X2RydmRhdGEocGRldiwgY2hpcCk7Cj4gKwo+ICsJcmV0 ID0gZGV2aWNlX2NyZWF0ZV9maWxlKCZwZGV2LT5kZXYsICZldWRfYXR0cmlidXRlKTsKPiArCWlm IChyZXQpCj4gKwkJcmV0dXJuIHJldDsKCllvdSBqdXN0IHJhY2VkIHdpdGggdXNlcnNwYWNlIGFu ZCBsb3N0IDooCgpJc24ndCB0aGVyZSBhIHdheSB0byBhZGQgdGhlIGF0dHJpYnV0ZSB0byB0aGUg cGxhdGZvcm0gZGV2aWNlIGFzIGEgZ3JvdXAKdG8gaGF2ZSBpdCBjb3JyZWN0bHkgYWRkZWQgYnkg dGhlIGRyaXZlciBjb3JlPwoKQWxzbywgeW91IGFyZSBhZGRpbmcgYW4gYXR0cmlidXRlIHRvIGEg c3RydWN0dXJlIHRoYXQgeW91IGRvIG5vdApjb250cm9sL293biwgYXJlIHlvdSBfc3VyZV8geW91 IHdhbnQgdG8gZG8gdGhhdD8gIEl0J3MgYSBiaXQgb2RkIGFuZCB5b3UKZG9uJ3QgcmVhbGx5IGNv bnRyb2wgdGhlIGxpZmV0aW1lIG9mIHRoYXQgZGV2aWNlLgoKCj4gKwo+ICsJY2hpcC0+ZXh0Y29u ID0gZGV2bV9leHRjb25fZGV2X2FsbG9jYXRlKCZwZGV2LT5kZXYsIGV1ZF9leHRjb25fY2FibGUp Owo+ICsJaWYgKElTX0VSUihjaGlwLT5leHRjb24pKQo+ICsJCXJldHVybiBQVFJfRVJSKGNoaXAt PmV4dGNvbik7CgpBcyBhbiBleGFtcGxlIG9mIHRoaXMgcHJvYmxlbSwgdGhlIHN5c2ZzIGZpbGUg aXMgbm93IHRoZXJlIGlmIHRoaXMgd2FzCmFuIGVycm9yLiAgWWV0IHRoYXQgc3lzZnMgZmlsZSBu b3cgbWVhbnMgbm90aGluZyBhbmQgaWYgdXNlcnNwYWNlCnRvdWNoZXMgaXQgYmFkIHRoaW5ncyB3 aWxsIGhhcHBlbi4KClNvIHBsZWFzZSBhdCB0aGUgdmVyeSBsZWFzdCwgcHJvcGVybHkgY2xlYW4g dXAgeW91ciBlcnJvciBwYXRocy4gIEFuZApsb29rIGludG8gZG9pbmcgdGhpcyBjb3JyZWN0bHku Cgp0aGFua3MsCgpncmVnIGstaAo=