[PATCH] kill tmscsim ->proc_info

[PATCH] kill tmscsim ->proc_info

[Christoph Hellwig]

hrrible code like all proc_info implementations and no really usefull
information.  We might want to add sysfs attributes if some users really
miss anything.


--- 1.45/drivers/scsi/tmscsim.c	2004-07-05 00:35:28 +02:00
+++ edited/drivers/scsi/tmscsim.c	2004-08-15 19:22:09 +02:00
@@ -295,9 +295,6 @@
 static int    dc390_initAdapter(struct Scsi_Host *psh, unsigned long io_port, u8 Irq, u8 index );
 static void   dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB);
 
-static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start,
-			    off_t offset, int length, int inout);
-
 static struct dc390_acb*	dc390_pACB_start= NULL;
 static struct dc390_acb*	dc390_pACB_current = NULL;
 static u32	dc390_laststatus = 0;
@@ -1664,7 +1630,6 @@
 static struct scsi_host_template driver_template = {
 	.module			= THIS_MODULE,
 	.proc_name		= "tmscsim", 
-	.proc_info		= DC390_proc_info,
 	.name			= DC390_BANNER " V" DC390_VERSION,
 	.slave_alloc		= dc390_slave_alloc,
 	.slave_configure	= dc390_slave_configure,
@@ -1784,152 +1749,6 @@
 	scsi_host_put(scsi_host);
 	pci_set_drvdata(dev, NULL);
 }
-
-/********************************************************************
- * Function: DC390_proc_info(char* buffer, char **start,
- *			     off_t offset, int length, int hostno, int inout)
- *
- * Purpose: return SCSI Adapter/Device Info
- *
- * Input: buffer: Pointer to a buffer where to write info
- *	  start :
- *	  offset:
- *	  hostno: Host adapter index
- *	  inout : Read (=0) or set(!=0) info
- *
- * Output: buffer: contains info
- *	   length; length of info in buffer
- *
- * return value: length
- *
- ********************************************************************/
-
-#undef SPRINTF
-#define SPRINTF(args...) pos += sprintf(pos, ## args)
-
-#define YESNO(YN)		\
- if (YN) SPRINTF(" Yes ");	\
- else SPRINTF(" No  ")
-
-
-static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start,
-			    off_t offset, int length, int inout)
-{
-  int dev, spd, spd1;
-  char *pos = buffer;
-  struct dc390_acb* pACB;
-  struct dc390_dcb* pDCB;
-
-  pACB = dc390_pACB_start;
-
-  while(pACB != (struct dc390_acb*)-1)
-     {
-	if (shpnt == pACB->pScsiHost)
-		break;
-	pACB = pACB->pNextACB;
-     }
-
-  if (pACB == (struct dc390_acb*)-1) return(-ESRCH);
-
-  if(inout) /* Has data been written to the file ? */
-      return -ENOSYS;
-   
-  SPRINTF("Tekram DC390/AM53C974 PCI SCSI Host Adapter, ");
-  SPRINTF("Driver Version %s\n", DC390_VERSION);
-
-  SPRINTF("SCSI Host Nr %i, ", shpnt->host_no);
-  SPRINTF("%s Adapter Nr %i\n", dc390_adapname, pACB->AdapterIndex);
-  SPRINTF("IOPortBase 0x%04x, ", pACB->IOPortBase);
-  SPRINTF("IRQ %02i\n", pACB->IRQLevel);
-
-  SPRINTF("MaxID %i, MaxLUN %i, ", shpnt->max_id, shpnt->max_lun);
-  SPRINTF("AdapterID %i, SelTimeout %i ms, DelayReset %i s\n", 
-	  shpnt->this_id, (pACB->sel_timeout*164)/100,
-	  dc390_eepromBuf[pACB->AdapterIndex][EE_DELAY]);
-
-  SPRINTF("TagMaxNum %i, Status 0x%02x, ACBFlag 0x%02x, GlitchEater %i ns\n",
-	  pACB->TagMaxNum, pACB->status, pACB->ACBFlag, GLITCH_TO_NS(pACB->glitch_cfg)*12);
-
-  SPRINTF("Statistics: Cmnds %li, Cmnds not sent directly %i, Out of SRB conds %i\n",
-	  pACB->Cmds, pACB->CmdInQ, pACB->CmdOutOfSRB);
-  SPRINTF("            Lost arbitrations %i, Sel. connected %i, Connected: %s\n", 
-	  pACB->SelLost, pACB->SelConn, pACB->Connected? "Yes": "No");
-   
-  SPRINTF("Nr of DCBs: %i\n", pACB->DCBCnt);
-  SPRINTF("Map of attached LUNs: %02x %02x %02x %02x %02x %02x %02x %02x\n",
-	  pACB->DCBmap[0], pACB->DCBmap[1], pACB->DCBmap[2], pACB->DCBmap[3], 
-	  pACB->DCBmap[4], pACB->DCBmap[5], pACB->DCBmap[6], pACB->DCBmap[7]);
-
-  SPRINTF("Idx ID LUN Prty Sync DsCn SndS TagQ NegoPeriod SyncSpeed SyncOffs MaxCmd\n");
-
-  pDCB = pACB->pLinkDCB;
-  for (dev = 0; dev < pACB->DCBCnt; dev++)
-     {
-      SPRINTF("%02i  %02i  %02i ", dev, pDCB->TargetID, pDCB->TargetLUN);
-      YESNO(pDCB->DevMode & PARITY_CHK_);
-      YESNO(pDCB->SyncMode & SYNC_NEGO_DONE);
-      YESNO(pDCB->DevMode & EN_DISCONNECT_);
-      YESNO(pDCB->DevMode & SEND_START_);
-      YESNO(pDCB->SyncMode & EN_TAG_QUEUEING);
-      if (pDCB->SyncOffset & 0x0f)
-      {
-	 int sp = pDCB->SyncPeriod; if (! (pDCB->CtrlR3 & FAST_SCSI)) sp++;
-	 SPRINTF("  %03i ns ", (pDCB->NegoPeriod) << 2);
-	 spd = 40/(sp); spd1 = 40%(sp);
-	 spd1 = (spd1 * 10 + sp/2) / (sp);
-	 SPRINTF("   %2i.%1i M      %02i", spd, spd1, (pDCB->SyncOffset & 0x0f));
-      }
-      else SPRINTF(" (%03i ns)                 ", (pDCB->NegoPeriod) << 2);
-      /* Add more info ...*/
-      SPRINTF ("      %02i\n", pDCB->MaxCommand);
-      pDCB = pDCB->pNextDCB;
-     }
-    if (timer_pending(&pACB->Waiting_Timer)) SPRINTF ("Waiting queue timer running\n");
-    else SPRINTF ("\n");
-    pDCB = pACB->pLinkDCB;
-	
-    for (dev = 0; dev < pACB->DCBCnt; dev++)
-    {
-	struct dc390_srb* pSRB;
-	if (pDCB->WaitSRBCnt) 
-		    SPRINTF ("DCB (%02i-%i): Waiting: %i:", pDCB->TargetID, pDCB->TargetLUN,
-			     pDCB->WaitSRBCnt);
-	for (pSRB = pDCB->pWaitingSRB; pSRB; pSRB = pSRB->pNextSRB)
-		SPRINTF(" %li", pSRB->pcmd->pid);
-	if (pDCB->GoingSRBCnt) 
-		    SPRINTF ("\nDCB (%02i-%i): Going  : %i:", pDCB->TargetID, pDCB->TargetLUN,
-			     pDCB->GoingSRBCnt);
-	for (pSRB = pDCB->pGoingSRB; pSRB; pSRB = pSRB->pNextSRB)
-#if 0 //def DC390_DEBUGTRACE
-		SPRINTF(" %s\n  ", pSRB->debugtrace);
-#else
-		SPRINTF(" %li", pSRB->pcmd->pid);
-#endif
-	if (pDCB->WaitSRBCnt || pDCB->GoingSRBCnt) SPRINTF ("\n");
-	pDCB = pDCB->pNextDCB;
-    }
-	
-#ifdef DC390_DEBUGDCB
-    SPRINTF ("DCB list for ACB %p:\n", pACB);
-    pDCB = pACB->pLinkDCB;
-    SPRINTF ("%p", pDCB);
-    for (dev = 0; dev < pACB->DCBCnt; dev++, pDCB=pDCB->pNextDCB)
-	SPRINTF ("->%p", pDCB->pNextDCB);
-    SPRINTF("\n");
-#endif
-  
-  *start = buffer + offset;
-
-  if (pos - buffer < offset)
-    return 0;
-  else if (pos - buffer - offset < length)
-    return pos - buffer - offset;
-  else
-    return length;
-}
-
-#undef YESNO
-#undef SPRINTF
 
 static struct pci_driver dc390_driver = {
 	.name           = "tmscsim",

Re: [PATCH] kill tmscsim ->proc_info

[Chiaki]

Christoph Hellwig wrote:
> hrrible code like all proc_info implementations and no really usefull
> information.  We might want to add sysfs attributes if some users really
> miss anything.
> 

Setting aside the horribleness of the code and suitability
of the code for 2.6.x kernel,
I found the information very useful when I did the debugging
with tmscsim driver code with
a rather obscure CD changer device and
other odd SCSI devices.

So I think adding sysfs attributes
would be a definite plus for 2.6.x series.

 From a happy user of tmscsi driver
under 2.4.xx series with HP DAT drive
and a PD/CD combo.

> 
> --- 1.45/drivers/scsi/tmscsim.c	2004-07-05 00:35:28 +02:00
> +++ edited/drivers/scsi/tmscsim.c	2004-08-15 19:22:09 +02:00
> @@ -295,9 +295,6 @@
>  static int    dc390_initAdapter(struct Scsi_Host *psh, unsigned long io_port, u8 Irq, u8 index );
>  static void   dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB);
>  
> -static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start,
> -			    off_t offset, int length, int inout);
> -
>  static struct dc390_acb*	dc390_pACB_start= NULL;
>  static struct dc390_acb*	dc390_pACB_current = NULL;
>  static u32	dc390_laststatus = 0;
> @@ -1664,7 +1630,6 @@
>  static struct scsi_host_template driver_template = {
>  	.module			= THIS_MODULE,
>  	.proc_name		= "tmscsim", 
> -	.proc_info		= DC390_proc_info,
>  	.name			= DC390_BANNER " V" DC390_VERSION,
>  	.slave_alloc		= dc390_slave_alloc,
>  	.slave_configure	= dc390_slave_configure,
> @@ -1784,152 +1749,6 @@
>  	scsi_host_put(scsi_host);
>  	pci_set_drvdata(dev, NULL);
>  }
> -
> -/********************************************************************
> - * Function: DC390_proc_info(char* buffer, char **start,
> - *			     off_t offset, int length, int hostno, int inout)
> - *
> - * Purpose: return SCSI Adapter/Device Info
> - *
> - * Input: buffer: Pointer to a buffer where to write info
> - *	  start :
> - *	  offset:
> - *	  hostno: Host adapter index
> - *	  inout : Read (=0) or set(!=0) info
> - *
> - * Output: buffer: contains info
> - *	   length; length of info in buffer
> - *
> - * return value: length
> - *
> - ********************************************************************/
> -
> -#undef SPRINTF
> -#define SPRINTF(args...) pos += sprintf(pos, ## args)
> -
> -#define YESNO(YN)		\
> - if (YN) SPRINTF(" Yes ");	\
> - else SPRINTF(" No  ")
> -
> -
> -static int DC390_proc_info (struct Scsi_Host *shpnt, char *buffer, char **start,
> -			    off_t offset, int length, int inout)
> -{
> -  int dev, spd, spd1;
> -  char *pos = buffer;
> -  struct dc390_acb* pACB;
> -  struct dc390_dcb* pDCB;
> -
> -  pACB = dc390_pACB_start;
> -
> -  while(pACB != (struct dc390_acb*)-1)
> -     {
> -	if (shpnt == pACB->pScsiHost)
> -		break;
> -	pACB = pACB->pNextACB;
> -     }
> -
> -  if (pACB == (struct dc390_acb*)-1) return(-ESRCH);
> -
> -  if(inout) /* Has data been written to the file ? */
> -      return -ENOSYS;
> -   
> -  SPRINTF("Tekram DC390/AM53C974 PCI SCSI Host Adapter, ");
> -  SPRINTF("Driver Version %s\n", DC390_VERSION);
> -
> -  SPRINTF("SCSI Host Nr %i, ", shpnt->host_no);
> -  SPRINTF("%s Adapter Nr %i\n", dc390_adapname, pACB->AdapterIndex);
> -  SPRINTF("IOPortBase 0x%04x, ", pACB->IOPortBase);
> -  SPRINTF("IRQ %02i\n", pACB->IRQLevel);
> -
> -  SPRINTF("MaxID %i, MaxLUN %i, ", shpnt->max_id, shpnt->max_lun);
> -  SPRINTF("AdapterID %i, SelTimeout %i ms, DelayReset %i s\n", 
> -	  shpnt->this_id, (pACB->sel_timeout*164)/100,
> -	  dc390_eepromBuf[pACB->AdapterIndex][EE_DELAY]);
> -
> -  SPRINTF("TagMaxNum %i, Status 0x%02x, ACBFlag 0x%02x, GlitchEater %i ns\n",
> -	  pACB->TagMaxNum, pACB->status, pACB->ACBFlag, GLITCH_TO_NS(pACB->glitch_cfg)*12);
> -
> -  SPRINTF("Statistics: Cmnds %li, Cmnds not sent directly %i, Out of SRB conds %i\n",
> -	  pACB->Cmds, pACB->CmdInQ, pACB->CmdOutOfSRB);
> -  SPRINTF("            Lost arbitrations %i, Sel. connected %i, Connected: %s\n", 
> -	  pACB->SelLost, pACB->SelConn, pACB->Connected? "Yes": "No");
> -   
> -  SPRINTF("Nr of DCBs: %i\n", pACB->DCBCnt);
> -  SPRINTF("Map of attached LUNs: %02x %02x %02x %02x %02x %02x %02x %02x\n",
> -	  pACB->DCBmap[0], pACB->DCBmap[1], pACB->DCBmap[2], pACB->DCBmap[3], 
> -	  pACB->DCBmap[4], pACB->DCBmap[5], pACB->DCBmap[6], pACB->DCBmap[7]);
> -
> -  SPRINTF("Idx ID LUN Prty Sync DsCn SndS TagQ NegoPeriod SyncSpeed SyncOffs MaxCmd\n");
> -
> -  pDCB = pACB->pLinkDCB;
> -  for (dev = 0; dev < pACB->DCBCnt; dev++)
> -     {
> -      SPRINTF("%02i  %02i  %02i ", dev, pDCB->TargetID, pDCB->TargetLUN);
> -      YESNO(pDCB->DevMode & PARITY_CHK_);
> -      YESNO(pDCB->SyncMode & SYNC_NEGO_DONE);
> -      YESNO(pDCB->DevMode & EN_DISCONNECT_);
> -      YESNO(pDCB->DevMode & SEND_START_);
> -      YESNO(pDCB->SyncMode & EN_TAG_QUEUEING);
> -      if (pDCB->SyncOffset & 0x0f)
> -      {
> -	 int sp = pDCB->SyncPeriod; if (! (pDCB->CtrlR3 & FAST_SCSI)) sp++;
> -	 SPRINTF("  %03i ns ", (pDCB->NegoPeriod) << 2);
> -	 spd = 40/(sp); spd1 = 40%(sp);
> -	 spd1 = (spd1 * 10 + sp/2) / (sp);
> -	 SPRINTF("   %2i.%1i M      %02i", spd, spd1, (pDCB->SyncOffset & 0x0f));
> -      }
> -      else SPRINTF(" (%03i ns)                 ", (pDCB->NegoPeriod) << 2);
> -      /* Add more info ...*/
> -      SPRINTF ("      %02i\n", pDCB->MaxCommand);
> -      pDCB = pDCB->pNextDCB;
> -     }
> -    if (timer_pending(&pACB->Waiting_Timer)) SPRINTF ("Waiting queue timer running\n");
> -    else SPRINTF ("\n");
> -    pDCB = pACB->pLinkDCB;
> -	
> -    for (dev = 0; dev < pACB->DCBCnt; dev++)
> -    {
> -	struct dc390_srb* pSRB;
> -	if (pDCB->WaitSRBCnt) 
> -		    SPRINTF ("DCB (%02i-%i): Waiting: %i:", pDCB->TargetID, pDCB->TargetLUN,
> -			     pDCB->WaitSRBCnt);
> -	for (pSRB = pDCB->pWaitingSRB; pSRB; pSRB = pSRB->pNextSRB)
> -		SPRINTF(" %li", pSRB->pcmd->pid);
> -	if (pDCB->GoingSRBCnt) 
> -		    SPRINTF ("\nDCB (%02i-%i): Going  : %i:", pDCB->TargetID, pDCB->TargetLUN,
> -			     pDCB->GoingSRBCnt);
> -	for (pSRB = pDCB->pGoingSRB; pSRB; pSRB = pSRB->pNextSRB)
> -#if 0 //def DC390_DEBUGTRACE
> -		SPRINTF(" %s\n  ", pSRB->debugtrace);
> -#else
> -		SPRINTF(" %li", pSRB->pcmd->pid);
> -#endif
> -	if (pDCB->WaitSRBCnt || pDCB->GoingSRBCnt) SPRINTF ("\n");
> -	pDCB = pDCB->pNextDCB;
> -    }
> -	
> -#ifdef DC390_DEBUGDCB
> -    SPRINTF ("DCB list for ACB %p:\n", pACB);
> -    pDCB = pACB->pLinkDCB;
> -    SPRINTF ("%p", pDCB);
> -    for (dev = 0; dev < pACB->DCBCnt; dev++, pDCB=pDCB->pNextDCB)
> -	SPRINTF ("->%p", pDCB->pNextDCB);
> -    SPRINTF("\n");
> -#endif
> -  
> -  *start = buffer + offset;
> -
> -  if (pos - buffer < offset)
> -    return 0;
> -  else if (pos - buffer - offset < length)
> -    return pos - buffer - offset;
> -  else
> -    return length;
> -}
> -
> -#undef YESNO
> -#undef SPRINTF
>  
>  static struct pci_driver dc390_driver = {
>  	.name           = "tmscsim",
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


-- 
int main(void){int j=2003;/*(c)2003 cishikawa. */
char t[] ="<CI> @abcdefghijklmnopqrstuvwxyz.,\n\"";
char *i ="g>qtCIuqivb,gCwe\np@.ietCIuqi\"tqkvv is>dnamz";
while(*i)((j+=strchr(t,*i++)-(int)t),(j%=sizeof t-1),
(putchar(t[j])));return 0;}/* under GPL */

Re: [PATCH] kill tmscsim ->proc_info

[Christoph Hellwig]

On Wed, Aug 18, 2004 at 01:46:38AM +0900, Chiaki wrote:
> Christoph Hellwig wrote:
> >hrrible code like all proc_info implementations and no really usefull
> >information.  We might want to add sysfs attributes if some users really
> >miss anything.
> >
> 
> Setting aside the horribleness of the code and suitability
> of the code for 2.6.x kernel,
> I found the information very useful when I did the debugging
> with tmscsim driver code with
> a rather obscure CD changer device and
> other odd SCSI devices.

What particular information?

Re: [PATCH] kill tmscsim ->proc_info

[Chiaki]

Christoph Hellwig wrote:
> On Wed, Aug 18, 2004 at 01:46:38AM +0900, Chiaki wrote:
> 
>>Christoph Hellwig wrote:
>>
>>>hrrible code like all proc_info implementations and no really usefull
>>>information.  We might want to add sysfs attributes if some users really
>>>miss anything.
>>>
>>
>>Setting aside the horribleness of the code and suitability
>>of the code for 2.6.x kernel,
>>I found the information very useful when I did the debugging
>>with tmscsim driver code with
>>a rather obscure CD changer device and
>>other odd SCSI devices.
> 
> 
> What particular information?
> 
> 

In the removed code section, there is a loop
that begins with
"for (dev = 0; dev < pACB->DCBCnt; dev++)".

I found the information printed by the code up to it
very informative.
Now my memory is getting hazy, but from the end user's view
point those are the informatin very useful.
A stupid user/administrator mistake like
forgetting to set lun search in the kernel is
made pretty obvious by the display.
(And I remember finding the problem with old linux
kernel code that the maximum limit of
added hard disk or removable devices was way too
low for modern PCs [even for home usage]. As a result of
input from users like me, the maximum added device limit
is increased very much if I recall correctly.)
Also, sometimes the available spec of disks and such
may not reflect the reality (the real shipped product
and the specification published may not agree,
or the difference between
the spec and the latest ROM information. By looking at the
display from the tmscsim module, I can learn the
real characteristics of the device as opposed to
the paper spec, and this was very useful for
a home user who doesn't have elaborate scsi inquiry tools.
Yes I know some free/non-free tools exist today, but
being able to look at such output of information
as understood by the driver itself, and sharing it
with the original author without requiring the
installation of additional tool when a problem occured was
very useful.

Sorry, I am not using 2.6.xx series kernel myself,
and writing sysfs support for the driver is beyond my
skill now.


I believe the output produced by the code below
the loop mentioned above would be
useful to the code maintainers who need to
learn what is going on (performance-wise, or
logical incorrectness, etc..), but I am not
sure on this part. (At one point during 2.2.x series,
a long time ago, I found an obscure list processing
problem possibly caused by the main kernel code.
But it was such a long time ago, I can't recall
the details now.)


-- 
int main(void){int j=2003;/*(c)2003 cishikawa. */
char t[] ="<CI> @abcdefghijklmnopqrstuvwxyz.,\n\"";
char *i ="g>qtCIuqivb,gCwe\np@.ietCIuqi\"tqkvv is>dnamz";
while(*i)((j+=strchr(t,*i++)-(int)t),(j%=sizeof t-1),
(putchar(t[j])));return 0;}/* under GPL */

Re: [PATCH] kill tmscsim ->proc_info

[Guennadi Liakhovetski]

On Tue, 17 Aug 2004, Christoph Hellwig wrote:

> hrrible code like all proc_info implementations and no really usefull
> information.  We might want to add sysfs attributes if some users really
> miss anything.

Well, I don't have a strong opinion on this one. I tend to believe you, 
Christoph, and I am sure James will take a wise decision:-) I might agree, 
that the code is ugly, but there was already at least 1 complaint:-)

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] kill tmscsim ->proc_info

[Christoph Hellwig]

On Wed, Aug 18, 2004 at 02:03:22AM +0900, Chiaki wrote:
> In the removed code section, there is a loop
> that begins with
> "for (dev = 0; dev < pACB->DCBCnt; dev++)".

We should get all the information back in sysfs once the driver is
changed to use the SPI transport class.