Re: [PATCH v2] [SCSI] dpt_i2o: use proper pci driver

[Johannes Thumshirn]

On Wed, Feb 17, 2016 at 09:20:03PM +0530, Sudip Mukherjee wrote:
> On Wednesday 17 February 2016 07:57 PM, One Thousand Gnomes wrote:
> >On Wed, 17 Feb 2016 17:50:14 +0530
> >Sudip Mukherjee  wrote:
> >
> >>This is a pci device but was not done in the usual way a pci driver is
> >>done. Convert the driver into a proper pci driver.
> >
> >This looks completely wrong. Please read the I2O 1.5 specification
> >document before playing with that code. You can't simply convert it to a
> >standard PCI driver as all the IOPs are supposed to be detected and then
> >the correct initialization sequence executed across the set of IOPs. IOPs
> >are allowed to talk to one another and the system table binds it all
> >together.
> 
> Yes, thanks for letting me know about the spec. It is saying
> "The host locates each IOP, adds it
> to the system configuration table, and initializes the IOP’s outbound
> message queue. The host then
> provides each IOP with a list of all IOPs and the physical location of their
> inbound message queue."
> 
> >
> >If you do hot plug then you need to follow the specification and do
> >all the extra notifications. Unless you've got multiple FC909/FC920
> >fibechannel cards or similar to test with I would just leave this well
> >alone.
> 
> point noted.
> 
> >Your original simple patch is *MUCH* safer in this specific case.
> 
> The original patch is already having NACK from Johannes. So i better leave
> this code here.

Nah, wasn't a hard NACK. I'm OK with the original code, just was reluctant to
the #ifdefs. I agree I didn't know the I2O specifics and thought a "normal"
PCI driver fitting nicely in the driver model would be the way to go.

Please resend your v1 with my Reviewed-by

> 
> regards
> sudip

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [SCSI] dpt_i2o: use proper pci driver

[Sudip Mukherjee]

On Wednesday 17 February 2016 07:57 PM, One Thousand Gnomes wrote:

On Wed, 17 Feb 2016 17:50:14 +0530
Sudip Mukherjee  wrote:


This is a pci device but was not done in the usual way a pci driver is
done. Convert the driver into a proper pci driver.


This looks completely wrong. Please read the I2O 1.5 specification
document before playing with that code. You can't simply convert it to a
standard PCI driver as all the IOPs are supposed to be detected and then
the correct initialization sequence executed across the set of IOPs. IOPs
are allowed to talk to one another and the system table binds it all
together.


Yes, thanks for letting me know about the spec. It is saying
"The host locates each IOP, adds it
to the system configuration table, and initializes the IOP’s outbound 
message queue. The host then
provides each IOP with a list of all IOPs and the physical location of 
their inbound message queue."




If you do hot plug then you need to follow the specification and do
all the extra notifications. Unless you've got multiple FC909/FC920
fibechannel cards or similar to test with I would just leave this well
alone.


point noted.


Your original simple patch is *MUCH* safer in this specific case.


The original patch is already having NACK from Johannes. So i better 
leave this code here.


regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [SCSI] dpt_i2o: use proper pci driver

[One Thousand Gnomes]

On Wed, 17 Feb 2016 17:50:14 +0530
Sudip Mukherjee  wrote:

> This is a pci device but was not done in the usual way a pci driver is
> done. Convert the driver into a proper pci driver.

This looks completely wrong. Please read the I2O 1.5 specification
document before playing with that code. You can't simply convert it to a
standard PCI driver as all the IOPs are supposed to be detected and then
the correct initialization sequence executed across the set of IOPs. IOPs
are allowed to talk to one another and the system table binds it all
together.

If you do hot plug then you need to follow the specification and do
all the extra notifications. Unless you've got multiple FC909/FC920
fibechannel cards or similar to test with I would just leave this well
alone. Your original simple patch is *MUCH* safer in this specific case.

So NAK

Alan


> 
> Signed-off-by: Sudip Mukherjee 
> ---
> 
> v1: only build warning related to "dptids defined but not used" was
> fixed using #ifdef
> 
>  drivers/scsi/dpt_i2o.c | 269 
> ++---
>  drivers/scsi/dpti.h|   5 +-
>  2 files changed, 120 insertions(+), 154 deletions(-)
> 
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index d4cda5e..9f50d1ae 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -111,6 +111,7 @@ static int sys_tbl_len;
>  
>  static adpt_hba* hba_chain = NULL;
>  static int hba_count = 0;
> +static bool control_reg;
>  
>  static struct class *adpt_sysfs_class;
>  
> @@ -187,118 +188,6 @@ static struct pci_device_id dptids[] = {
>  };
>  MODULE_DEVICE_TABLE(pci,dptids);
>  
> -static int adpt_detect(struct scsi_host_template* sht)
> -{
> - struct pci_dev *pDev = NULL;
> - adpt_hba *pHba;
> - adpt_hba *next;
> -
> - PINFO("Detecting Adaptec I2O RAID controllers...\n");
> -
> -/* search for all Adatpec I2O RAID cards */
> - while ((pDev = pci_get_device( PCI_DPT_VENDOR_ID, PCI_ANY_ID, pDev))) {
> - if(pDev->device == PCI_DPT_DEVICE_ID ||
> -pDev->device == PCI_DPT_RAPTOR_DEVICE_ID){
> - if(adpt_install_hba(sht, pDev) ){
> - PERROR("Could not Init an I2O RAID device\n");
> - PERROR("Will not try to detect others.\n");
> - return hba_count-1;
> - }
> - pci_dev_get(pDev);
> - }
> - }
> -
> - /* In INIT state, Activate IOPs */
> - for (pHba = hba_chain; pHba; pHba = next) {
> - next = pHba->next;
> - // Activate does get status , init outbound, and get hrt
> - if (adpt_i2o_activate_hba(pHba) < 0) {
> - adpt_i2o_delete_hba(pHba);
> - }
> - }
> -
> -
> - /* Active IOPs in HOLD state */
> -
> -rebuild_sys_tab:
> - if (hba_chain == NULL) 
> - return 0;
> -
> - /*
> -  * If build_sys_table fails, we kill everything and bail
> -  * as we can't init the IOPs w/o a system table
> -  */ 
> - if (adpt_i2o_build_sys_table() < 0) {
> - adpt_i2o_sys_shutdown();
> - return 0;
> - }
> -
> - PDEBUG("HBA's in HOLD state\n");
> -
> - /* If IOP don't get online, we need to rebuild the System table */
> - for (pHba = hba_chain; pHba; pHba = pHba->next) {
> - if (adpt_i2o_online_hba(pHba) < 0) {
> - adpt_i2o_delete_hba(pHba);  
> - goto rebuild_sys_tab;
> - }
> - }
> -
> - /* Active IOPs now in OPERATIONAL state */
> - PDEBUG("HBA's in OPERATIONAL state\n");
> -
> - printk("dpti: If you have a lot of devices this could take a few 
> minutes.\n");
> - for (pHba = hba_chain; pHba; pHba = next) {
> - next = pHba->next;
> - printk(KERN_INFO"%s: Reading the hardware resource table.\n", 
> pHba->name);
> - if (adpt_i2o_lct_get(pHba) < 0){
> - adpt_i2o_delete_hba(pHba);
> - continue;
> - }
> -
> - if (adpt_i2o_parse_lct(pHba) < 0){
> - adpt_i2o_delete_hba(pHba);
> - continue;
> - }
> - adpt_inquiry(pHba);
> - }
> -
> - adpt_sysfs_class = class_create(THIS_MODULE, "dpt_i2o");
> - if (IS_ERR(adpt_sysfs_class)) {
> - printk(KERN_WARNING"dpti: unable to create dpt_i2o class\n");
> - adpt_sysfs_class = NULL;
> - }
> -
> - for (pHba = hba_chain; pHba; pHba = next) {
> - next = pHba->next;
> - if (adpt_scsi_host_alloc(pHba, sht) < 0){
> - adpt_i2o_delete_hba(pHba);
> - continue;
> - }
> - pHba->initialized = TRUE;
> - pHba->state &= ~DPTI_STATE_RESET;
> - if (adpt_sysfs_class) {
> -  

[PATCH v2] [SCSI] dpt_i2o: use proper pci driver

[Sudip Mukherjee]

This is a pci device but was not done in the usual way a pci driver is
done. Convert the driver into a proper pci driver.

Signed-off-by: Sudip Mukherjee 
---

v1: only build warning related to "dptids defined but not used" was
fixed using #ifdef

 drivers/scsi/dpt_i2o.c | 269 ++---
 drivers/scsi/dpti.h|   5 +-
 2 files changed, 120 insertions(+), 154 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index d4cda5e..9f50d1ae 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -111,6 +111,7 @@ static int sys_tbl_len;
 
 static adpt_hba* hba_chain = NULL;
 static int hba_count = 0;
+static bool control_reg;
 
 static struct class *adpt_sysfs_class;
 
@@ -187,118 +188,6 @@ static struct pci_device_id dptids[] = {
 };
 MODULE_DEVICE_TABLE(pci,dptids);
 
-static int adpt_detect(struct scsi_host_template* sht)
-{
-   struct pci_dev *pDev = NULL;
-   adpt_hba *pHba;
-   adpt_hba *next;
-
-   PINFO("Detecting Adaptec I2O RAID controllers...\n");
-
-/* search for all Adatpec I2O RAID cards */
-   while ((pDev = pci_get_device( PCI_DPT_VENDOR_ID, PCI_ANY_ID, pDev))) {
-   if(pDev->device == PCI_DPT_DEVICE_ID ||
-  pDev->device == PCI_DPT_RAPTOR_DEVICE_ID){
-   if(adpt_install_hba(sht, pDev) ){
-   PERROR("Could not Init an I2O RAID device\n");
-   PERROR("Will not try to detect others.\n");
-   return hba_count-1;
-   }
-   pci_dev_get(pDev);
-   }
-   }
-
-   /* In INIT state, Activate IOPs */
-   for (pHba = hba_chain; pHba; pHba = next) {
-   next = pHba->next;
-   // Activate does get status , init outbound, and get hrt
-   if (adpt_i2o_activate_hba(pHba) < 0) {
-   adpt_i2o_delete_hba(pHba);
-   }
-   }
-
-
-   /* Active IOPs in HOLD state */
-
-rebuild_sys_tab:
-   if (hba_chain == NULL) 
-   return 0;
-
-   /*
-* If build_sys_table fails, we kill everything and bail
-* as we can't init the IOPs w/o a system table
-*/ 
-   if (adpt_i2o_build_sys_table() < 0) {
-   adpt_i2o_sys_shutdown();
-   return 0;
-   }
-
-   PDEBUG("HBA's in HOLD state\n");
-
-   /* If IOP don't get online, we need to rebuild the System table */
-   for (pHba = hba_chain; pHba; pHba = pHba->next) {
-   if (adpt_i2o_online_hba(pHba) < 0) {
-   adpt_i2o_delete_hba(pHba);  
-   goto rebuild_sys_tab;
-   }
-   }
-
-   /* Active IOPs now in OPERATIONAL state */
-   PDEBUG("HBA's in OPERATIONAL state\n");
-
-   printk("dpti: If you have a lot of devices this could take a few 
minutes.\n");
-   for (pHba = hba_chain; pHba; pHba = next) {
-   next = pHba->next;
-   printk(KERN_INFO"%s: Reading the hardware resource table.\n", 
pHba->name);
-   if (adpt_i2o_lct_get(pHba) < 0){
-   adpt_i2o_delete_hba(pHba);
-   continue;
-   }
-
-   if (adpt_i2o_parse_lct(pHba) < 0){
-   adpt_i2o_delete_hba(pHba);
-   continue;
-   }
-   adpt_inquiry(pHba);
-   }
-
-   adpt_sysfs_class = class_create(THIS_MODULE, "dpt_i2o");
-   if (IS_ERR(adpt_sysfs_class)) {
-   printk(KERN_WARNING"dpti: unable to create dpt_i2o class\n");
-   adpt_sysfs_class = NULL;
-   }
-
-   for (pHba = hba_chain; pHba; pHba = next) {
-   next = pHba->next;
-   if (adpt_scsi_host_alloc(pHba, sht) < 0){
-   adpt_i2o_delete_hba(pHba);
-   continue;
-   }
-   pHba->initialized = TRUE;
-   pHba->state &= ~DPTI_STATE_RESET;
-   if (adpt_sysfs_class) {
-   struct device *dev = device_create(adpt_sysfs_class,
-   NULL, MKDEV(DPTI_I2O_MAJOR, pHba->unit), NULL,
-   "dpti%d", pHba->unit);
-   if (IS_ERR(dev)) {
-   printk(KERN_WARNING"dpti%d: unable to "
-   "create device in dpt_i2o class\n",
-   pHba->unit);
-   }
-   }
-   }
-
-   // Register our control device node
-   // nodes will need to be created in /dev to access this
-   // the nodes can not be created from within the driver
-   if (hba_count && register_chrdev(DPTI_I2O_MAJOR, DPT_DRIVER, 
_fops)) {
-   adpt_i2o_sys_shutdown();
-   return 0;
-   }
-   return hba_count;